2
\$\begingroup\$

I am building a string for a search query and I think I am doing it the wrong way.

So basically I have serveral textfields, dates, integers and so on in one form. On submit I am checking the forms if its empty or not and building the query string.

I am submitting the model and building a kind of "pyramid" by checking if it is null or not, which is ending in a very large if statement (marked emphasis).

public string CreateSearchQuery(SearchViewModel model)
    {
        StringBuilder buildQueryString = new StringBuilder(2048);

        if (model == null)
        {
            //empty model: return newest results
            return buildQueryString.Append("SELECT * FROM newTbl.files").ToString();
        }

        buildQueryString.Append("SELECT * FROM newTbl.files INNER JOIN newTbl.folders ON folders.id = f_folders_id WHERE ");

        if (model.NumberClubs > 0)
        {
            buildQueryString.Append("folders.numberClubs = ");
            buildQueryString.Append(model.NumberClubs);
        }

        if(model.MrDateFrom != null)
        {
             //those if statements will get very large, when I have more then ten fields
            *if (model.NumberClubs > 0)*
            {
                buildQueryString.Append(" AND ");
            }
            buildQueryString.Append("folders.mrDate = '");
            buildQueryString.Append(model.MrDateFrom.Value.ToString("dd/MM/yyyy"));
            buildQueryString.Append("'");
        }

        if (model.Period > 0)
        {
            //those if statements will get very large, when I have more then ten fields
            *if (model.NumberClubs > 0 || model.MrDateFrom != null)*
            {
                buildQueryString.Append(" AND ");
            }
            buildQueryString.Append("folders.period = ");
            buildQueryString.Append(model.Period);
        }

        buildQueryString.Append(" ORDER BY newTbl.period DESC");

        return buildQueryString.ToString();
    }

I don't think that this is the correct way. So I have a model with several values, but to build a query string I have to check if it is empty or not to append the AND? I mean it is working, but it is really complicated (in my opinion).

\$\endgroup\$
  • 4
    \$\begingroup\$ Is there any particular reason your are not using SqlParameter? \$\endgroup\$ – t3chb0t Jan 28 at 17:52
  • \$\begingroup\$ @t3chb0t Yes, our aim is first use stringbuilder, then use NpgsqlParameter (Postgres) and after that we should use Dapper. To see the different things (pros & cons - so also SQL Injection) and how it is working. \$\endgroup\$ – Spedo De La Rossa Jan 29 at 6:35
2
\$\begingroup\$

As t3chb0t suggested, when creating SQL based on user input use ALWAYS SqlParameters to prevent sql injection!

Instead of creating SQL using a StringBuilder, I would try to abstract common structures and create classes to represent them. That makes the code more readable and maintainable.

A more structured solution could look like that:

    private class Condition
    {
        public Condition(string field, string parameterName, object paramerterValue)
        {
            this.Field = field;
            this.ParameterName = parameterName;
            this.ParameterValue = paramerterValue;
        }

        public string Field { get; }
        public string ParameterName { get; }
        public object ParameterValue { get; }

        public override string ToString() => $"{Field} = @{ParameterValue}";
    }

    public class SqlQuery
    {
        public SqlQuery(string sql, SqlParameter[] parameters = null)
        {
            this.Sql = sql;
            this.SqlParameters = parameters ?? new SqlParameter[0];
        }

        public string Sql { get; }
        public SqlParameter[] SqlParameters { get; }
    }

    public SqlQuery CreateSearchQuery(SearchViewModel model)
    {
        if (model == null) return new SqlQuery("SELECT * FROM newTbl.files");

        var sql = "SELECT * FROM newTbl.files fi INNER JOIN newTbl.folders fo ON fo.id = fi.f_folders_id";

        var conditions = new List<Condition>();
        var sqlParameters = new SqlParameter[0];
        var pcount = 1;

        if (model.NumberClubs > 0) conditions.Add(new Condition("fo.numberClubs", $"p{pcount++}", model.NumberClubs));
        if (model.MrDateFrom != null) conditions.Add(new Condition("fo.mrDate", $"p{pcount++}", model.MrDateFrom.Value));
        if (model.Period > 0) conditions.Add(new Condition("fo.period", $"p{pcount++}", model.Period));

        if (conditions.Count > 0)
        {
            var whereClause = " WHERE " + string.Join(" AND ", conditions);
            sqlParameters = conditions.Select(c => new SlqParameter(c.ParameterName, c.ParameterValue)).ToArray();
            sql += whereClause;
        }

        sql += " ORDER BY newTbl.period DESC";

        return new SqlQuery(sql, sqlParameters);
    }
\$\endgroup\$
  • \$\begingroup\$ I answered techbot. ;) Yes, there is a reason, to see how the stuff is working/changing. So my question was about the if clauses. What is $"p{pcount++}". I am new to C#, but I don't know this syntax. If I am right, it should be the parameterName? Yes, it is easier to read, but when I have 10+ textfields, i will came up with +10 if statements? Is that ok or bad? \$\endgroup\$ – Spedo De La Rossa Jan 29 at 6:43

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service, privacy policy and cookie policy

Not the answer you're looking for? Browse other questions tagged or ask your own question.