2

Consider the following function which has 2 optional variables

public List<T> SelectSqlItems<T>(
    string settingsgroup = null,
    int? state = null)
{
    SqlCommand selectCommand = null;

    if (settingsgroup == null)
    {
        selectCommand = new SqlCommand(
            "select * from ApplicationSettings ", con);
    }
    else
    {
        selectCommand = new SqlCommand(
            string.Format(
                "select * from ApplicationSettings where settingsgroup='{0}' ",
                settingsgroup),
            con);
    }

    if (state != null)
    {
        selectCommand.CommandText += 
            !selectCommand
                .CommandText
                .ToLower()
                .Contains("where")
            ? string.Format("where state={0}", state)
            : string.Format("and state={0}", state);
    }

    //etc..
}

I have 4 possibilities:

settingsgroup==null && state==null
settingsgroup==null && state!=null
settingsgroup!=null && state==null
settingsgroup!=null && state!=null

From every case above a different SQL command has to be produced. What are the built in functionalities in C# that could help me achieve such things without a lot of conditional statements, and if you were to write the above how would you write it other than having to overload the function 4 times?

Brett Wolfington
  • 6,587
  • 4
  • 32
  • 51
FPGA
  • 3,525
  • 10
  • 44
  • 73

5 Answers5

10

This is a common problem in SQL that can be effectively handled in the query itself, thus allowing queries to be created in advance, use parameters, and be accessed through stored procedures.

Use of parameters is an important recommendation and should not be considered optional. SQL Parameters will help prevent SQL injection attacks. For example, imagine if someone were to call your method using the following parameter values:

SelectSqlItems<T>("' OR settingsgroup <> '", null);

Your query would now become:

select * from ApplicationSettings where settingsgroup='' OR settingsgroup<>'' 

This would of course return all rows from the table, and potentially expose private information. Even worse possibilities exist, however, such as inserting a DELETE clause which could delete your whole table, or even drop your entire database (though hopefully your user permissions are configured to at least prevent these worst-case scenarios).

To prevent this, your SelectSqlItems method can be restated to the following:

public List<T> SelectSqlItems<T>(
    string settingsgroup = null,
    int? state = null)
{
    var cmdText = "..."; // See Query Below
    var selectCommand = new SqlCommand(cmdText, con);

    // Set the values of the parameters
    selectCommand.Parameters.AddWithValue("@settingsgroup", settingsgroup);
    selectCommand.Parameters.AddWithValue("@state", state);

    // etc...
}

Your query can now be stated as follows:

SELECT
    *
FROM
    ApplicationSettings
WHERE
    ((@settingsgroup IS NULL) OR (settingsgroup=@settingsgroup))
    AND
    ((@state IS NULL) OR (state=@state)) 

If a parameter value is null, the left side of the conditional statement joined by OR will always have the value TRUE, and therefore all rows will be matched. If, however, the parameter value is not NULL, the left side of the conditional will have the value FALSE and the right side will be inspected. The right side will only have the value TRUE if the row's value matches the parameter value, and therefore only the rows matching the parameter value will be returned. This concept can be repeated with as many parameters as required.

Brett Wolfington
  • 6,587
  • 4
  • 32
  • 51
3

Why not switch to an SQL stored procedure with both parameters being optional and pass the parameters passed to SelectSqlItems directly to it ?

Community
  • 1
  • 1
Francis Ducharme
  • 4,848
  • 6
  • 43
  • 81
2

If you switched to a ORM solution like Entity Framework you could dynamically build your query with functions easily.

public List<T> SelectSqlItems<T>(string settingsgroup=null,int? state=null)
{
    using(var context = new MyContext())
    {
         IQueyable<ApplicationSettings> query = context.ApplicationSettings;

         if(settingsgroup != null)
             query = query.Where(row => row.settingsgroup = settingsgroup);

         if(state != null)
             query = query.Where(row => row.state = state.Value)

         Expression<Func<ApplicationSettings, T>> selectExpression = GetSelectExpression<T>();

         return query.Select(selectExpression).ToList();
    }
}
Scott Chamberlain
  • 124,994
  • 33
  • 282
  • 431
1

This would probably work. It also won't enforce null if a parameter is not passed in. You should look into using Parameters if you are concerned about injection attacks. This is not a safe way to add parameters to a query.

        string stateCompare = state.HasValue ? "state = " + state.Value : "";
        string settingsgroupCompare = String.IsNullOrEmpty(settingsgroup) ? "IS NULL" : "= " + settingsgroup;
        string whereCondition = !String.IsNullOrEmpty(stateCompare) || !String.IsNullOrEmpty(settingsgroupCompare)?"WHERE":"";
        SqlCommand selectCommand = new SqlCommand(String.Format("select * from ApplicationSettings {0} {1} {2}",whereCondition, settingsgroupCompare, stateCompare);
AFrieze
  • 844
  • 1
  • 10
  • 26
1

Mandatory SQL injection warning: Do not use string constants originating in the user's input directly. Parameterize your queries.

If you insist on building a SQL statement dynamically (as opposed to having it built by one of the built-in or open-source ORM solutions available in .NET), you could either simplify your code by using a fake WHERE 1=1 condition, a common trick of dynamic SQL builders, or by "encoding" the states as numbers, and processing them in a switch.

The "trick" solution starts like this:

if (settingsgroup == null) {
    selectCommand = new SqlCommand("select * from ApplicationSettings WHERE 1=1 ", con);
} else {
    selectCommand = new SqlCommand(string.Format("select * from ApplicationSettings where settingsgroup='{0}' ", settingsgroup), con);
}

This looks similar to what you have, except that you no longer need to check the existing string for presence or absence of the WHERE clause: it's always there. You can continue with your simplified code:

if (state != null) {
    selectCommand.CommandText += string.Format("and state={0}",state);
} ... // and so on

An alternative would be "encoding" the state explicitly in a state number, and using it in a switch, like this:

int conditionForm = 0;
if (settingsgroup != 0) conditionForm |= 1;
if (state != 0) conditionForm |= 2; // Use powers of two

Now the conditionForm variable can have one of four values from the range 0..3, inclusive. You can write a switch statement that deals with each condition separately:

switch (conditionForm) {
    case 0:
         selectCommand = new SqlCommand("select * from ApplicationSettings", con);
         break;
    case 1:
         selectCommand = new SqlCommand(string.Format("select * from ApplicationSettings where settingsgroup='{0}'", settingsgroup), con);
         break;
    case 2:
         selectCommand = new SqlCommand(string.Format("select * from ApplicationSettings where state='{0}'", state), con);
         break;
    case 3:
         selectCommand = new SqlCommand(string.Format("select * from ApplicationSettings where settingsgroup='{0}' and state='{1}'", settingsgroup, state), con);
         break;
}
Sergey Kalinichenko
  • 714,442
  • 84
  • 1,110
  • 1,523