Minimise the number of conditions

Question

protected void Button_Click(object sender, EventArgs e)
{
    string name = this.name.Text;
    string age = this.age.Text;
    string state = this.state.Text;
    string classes = this.classes.Text;

    string query = "select StudentInfoId, Name, Age, State, tbl2.ClassName from tblstudentinfo tbl1 join Class tbl2 on (tbl1.class = tbl2.classid)";

    if (name != "" || age != "" || state != "" || classes != "")
    {
        query += " where";
    }

    if (name != "")
    {
        query = query + " name = '" + name + "' " ;
    }

    if (age != "")
    {
        if (name != "")
        {
            query = query + " and age = '" + age + "' ";
        }
        else
        {
            query = query + " age = '" + age + "' ";
        }
    }

    if (state != "")
    {
        if (name != "" || age != "")
        {
            query = query + " and state = '" + state + "' ";
        }
        else
        {
            query = query + " state = '" + state + "' ";
        }
    }

    if (classes != "")
    {
        if (name != "" || age != "" || state != "")
        {
            query = query + " and class = '" + classes + "' ";
        }
        else
        {
            query = query + " class = '" + classes + "' ";
        }
    }
    BindData(query);
}


private void BindData(string query)
{
    SqlDataAdapter da = new SqlDataAdapter(query, conn);
    DataSet ds = new DataSet();
    da.Fill(ds);
    GridView1.DataSource = ds;
    GridView1.DataBind();
}

You should post your question on [CodeReview](http://codereview.stackexchange.com/) (and properly format it). — Patrice Gahide, Oct 16 '14 at 08:32
You are open for sql-injection. You should use sql-parameters and the correct data types instead of string concatenation. — Tim Schmelter, Oct 16 '14 at 08:38
**[USE PARAMEREISED QUERIES](http://bobby-tables.com/)** It allows for query plan caching so avoids recompiling your query with every exection, it allows for typed parameters to avoid conversions, and protects you against malformed and malicious sql. — GarethD, Oct 16 '14 at 08:42
This question appears to be off-topic because it is about code that is already working properly. As suggested by others, it should be migrated to [CodeReview.SE]. — S.L. Barth is on codidact.com, Oct 16 '14 at 08:49
Besides solving the SQL injection issues, it would also be better to [use a using statement with the SqlDataAdapter](http://stackoverflow.com/questions/15333522/sqldataadapter-with-using-keyword). — BCdotWEB, Oct 16 '14 at 08:55

Tim Schmelter · Answer 1 · 2014-10-16T09:06:47.687

Actually you are trying to optimize the wrong thing. You should not use string concatenation to add columns and values at all. You are vulnerable to sql-injection and also treating all data types as string which can cause other issues.

Instead you should use sql-parameters. Here is an example:

string query = @"SELECT StudentInfoId, Name, Age, State, tbl2.ClassName 
                 FROM   tblstudentinfo tbl1 
                 INNER JOIN Class tbl2 ON (tbl1.class = tbl2.classid)
                 WHERE (@name IS NULL OR name = @name)
                 AND   (@age IS NULL OR age = @age)
                 AND   (@state IS NULL OR state = @state)
                 AND   (@classes IS NULL OR classes = @classes)";

DataTable table = new DataTable();
using(var conn = new SqlConnection(" ... "))
using(var da = new SqlDataAdapter(query, conn))
{
    var parameters = da.SelectCommand.Parameters;
    var p = new SqlParameter("@name", SqlDbType.NVarChar);
    if(!string.IsNullOrWhiteSpace(this.name.Text))
        p.Value = this.name.Text.Trim();
    else
        p.Value = DBNull.Value;
    parameters.Add(p);

    p = new SqlParameter("@age", SqlDbType.Int);
    int age;
    if(int.TryParse(this.age.Text, out age))
        p.Value = age;
    else
        p.Value = DBNull.Value;
    parameters.Add(p);

    // ...

    da.Fill(table);
}

GridView1.DataSource = table;
GridView1.DataBind();

+1 Although I'd be inclined to add `OPTION (RECOMPILE)` to the query to allow indexes to be used where relevant, and redundant predicates to be optimised away. — GarethD, Oct 16 '14 at 09:05
@GarethD: you're right, it can be improved. But i just wanted to show a way to support a "one filter for all" without too much "noise". If the table is not too large it might be fast enough. Worth reading: http://blogs.msdn.com/b/bartd/archive/2009/05/03/sometimes-the-simplest-solution-isn-t-the-best-solution-the-all-in-one-search-query.aspx — Tim Schmelter, Oct 16 '14 at 09:11

score 0 · Answer 2 · answered Oct 16 '14 at 08:34

Try the following. I just added where 1=1 to the default query, so when adding other parts of the query you would always need the and.

string query = "select StudentInfoId, Name, Age, State, tbl2.ClassName from tblstudentinfo tbl1 join Class tbl2 on (tbl1.class = tbl2.classid) where 1=1 ";


    if (name != "")
    {
        query = query + " and name = '" + name + "' " ;
    }

    if (age != "")
    {
         query = query + " and age = '" + age + "' ";
    }

    if (state != "")
    {
          query = query + " and state = '" + state + "' ";
    }

    if (classes != "")
    {
          query = query + " and class = '" + classes + "' ";
    }

While this is an improvement on the original code, it is vulnerable to SQL injection. Instead, [use parameters](http://www.dotnetperls.com/sqlparameter). — BCdotWEB, Oct 16 '14 at 08:42

Minimise the number of conditions

2 Answers2