How can I choose the right string with fewer lines and less repeating

Question

First of all I know my code is highly vulnerable to SQL injections, but I wanted to code the base first. I have a feeling this can be done with fewer lines and less repeating. I'm sorry if it is really bad code I just started learning C#.

public DataTable GetData(string search, int filter)
        {
            switch (filter)
            {
                case 0:
                    sqlQuery = string.Format("SELECT * FROM movies WHERE movie_id = {0} OR movie_name = \"{0}\" OR movie_director = \"{0}\";", search);
                    break;
                case 1:
                    sqlQuery = string.Format("SELECT * FROM movies WHERE move_id = {0};", search);
                    break;
                case 2:
                    sqlQuery = string.Format("SELECT * FROM movies WHERE movie_name = \"{0}\"", search);
                    break;
                case 3:
                    sqlQuery = string.Format("SELECT * FROM movies WHERE movie_director = \"{0}\";", search);
                    break;
                default:
                    sqlQuery = "SELECT * FROM movies";
                    break;
            }

Answer 1

First of all I know my code is highly vulnerable to SQL injections, but I wanted to code the base first.

No, you don't. You really don't want to do that. I was going to leave this as a comment, but it's too extensive and too important for that:

Security in a program should be incorporated before making optimizations. With every security feature added you risk breaking those optimizations open again, rendering all work you put into them null and void.

Make it work first, make it pretty later. In this case, I'd definitely consider adding the security a mandatory feature of the code. SQL injection has been around for a long time and the single reason it's still a thing is because code like yours is still around. Nobody wants that, really.

In this case, you'll find parametrized queries will solve your injection problem. However, those can't be optimized like ordinary strings. They got their own rules and tricks.

Answer 2

You could encapsulate the variable parts in a separated class.

The solution has more lines, but it complies with the open/closed principle. Therefore the logic can be simply extended without changing a switch statement.

public enum FilterType
{
    All = 0,
    ID = 1,
    Name = 2,
    Director = 3
}

public class Filter
{ 
    private readonly FilterType_filterType;
    private readonly string _constraint;

    public Filter(FilterType filterType, string constraint)
    {
        _filterType = filterType;
        _constraint = constraint;
    }

    public FilterType FilterType => _filterType;
    public string GetWhereClause(string value) => string.Format(constraint, value);
}

[...]

var filters = new[]
{
    new Filter(FilterType.All, "movie_id = {0} OR movie_name = {0} OR movie_director = {0}"),
    new Filter(FilterType.ID, "move_id = {0}"),
    new Filter(FilterType.Name, "movie_name = {0}"),
    new Filter(FilterType.Director, "movie_director = {0}"),
}

[...]

public string GetSql(string search, FilterType filterType)
{

    var filter = filters.FirstOrDefault(f => f.FilterType = filterType);
    var whereClause = filter == null ? "" : filter.GetWhereClause(search);

    return "SELECT * FROM film_films " + whereClause + ";";
}

Answer 3

With a few changes you could create the queries dynamically. In order to do this you need to define an enum decorated with the FlagsAttriubte and specify the column names as custom attributes:

[Flags]
public enum FilterColumns
{
    [FilterColumn("movie_id")]
    Id = 1,

    [FilterColumn("movie_name")]
    Name = 2,

    [FilterColumn("movie_director")]
    Director = 4,
}

The FilterColumnAttribute could be implemented like this:

[AttributeUsage(AttributeTargets.Field)]
public class FilterColumnAttribute : Attribute
{
    private readonly string _name;

    public FilterColumnAttribute(string name) { _name = name; }

    public override string ToString() { return _name; }

    public static implicit operator string (FilterColumnAttribute attr) { return attr.ToString(); }
}

To read the column names te Filter needs a little bit of reflection:

public class Filter
{
    public Filter(FilterColumns filterColumns)
    {
        FilterColumns = filterColumns;
        var flags = Enum.GetValues(typeof(FilterColumns))
            .Cast<FilterColumns>()
            .Where(flag => filterColumns.HasFlag(flag));
        Constraint = string.Join(" OR ", flags
            .Select(flag => string.Format(
                "{0} = \"{{0}}\"", 
                typeof(FilterColumns)
                    .GetField(flag.ToString())
                    .GetCustomAttribute(typeof(FilterColumnAttribute)))));
    }

    public FilterColumns FilterColumns { get; }

    public string Constraint { get; }

    public string GetWhereClause(string value) { ... }
}

but now you can combine the filters in any way by using the appropriate flag:

var filters = new[]
{
    new Filter(FilterColumns.Id | FilterColumns.Name | FilterColumns.Director),
    new Filter(FilterColumns.Id),
    new Filter(FilterColumns.Name),
    new Filter(FilterColumns.Director),
};

Answer 4

Forgive me for not providing a complete answer with examples (due to the lengthy setup), but this seems like a good time to research and implement a repository pattern, and combine it with something like Nhibernate & Linq.

Repository pattern: https://msdn.microsoft.com/en-us/library/ff649690.aspx Nhibernate & Linq tutorial: https://www.tutorialspoint.com/nhibernate/nhibernate_linq.htm

This would enable you to query your database with the following (based on your examples) -

repository.Query<Movie>().Where(movie => movie.ID == id);

Going a step further, you can then create services which wrap your repository calls for your entities (e.g. MovieService) that take your search criterias as parameters.

If this is all for the purpose of learning, then I would take the extra time to learn the patterns/ORMs/Linq. Please do ask if you need further guidance.