Improving code readability for SQL commands

Question

In one of my classes for a web application I am developing, I have some reasonably long SQL queries.

When developing a three-tier application, what are the best practices to make this kind of code neater?

Dim dc As New SqlCommand("INSERT INTO Choices VALUES ('" + _
                                 SanitizeInput(strUser) + "', '" + _
                                 SanitizeInput(strFirstHalfDay) + "', '" + _
                                 SanitizeInput(strSecondHalfDay) + "', '" + _
                                 SanitizeInput(strFullDay) + "', " + _
                                 SanitizeInput(Convert.ToInt32(firstHalfPaid).ToString()) + ", " + _
                                 SanitizeInput(Convert.ToInt32(secondHalfPaid).ToString()) + ", " + _
                                 SanitizeInput(Convert.ToInt32(fullPaid).ToString()) + ")", cn)

Do you consider this kind of code to be acceptable or stinky?

Answer 1

STOP, don't do that, use prepared staments, you'll get security and readability.

Use this instead:

Dim dc As New SqlCommand("INSERT INTO Choices VALUES (@User, @FirstHalfDay, @SecondHalfDay, @FullDay, @FirstHalfPaid, @SecondHalfPaid, @FullPaid'", cn)
dc.Parameters.Add (new SqlParameter ("User", strUser))
dc.Parameters.Add (new SqlParameter ("FirstHalfDay", strFirstHalfDay))
dc.Parameters.Add (new SqlParameter ("SecondHalfDay", strSecondHalfDay))
dc.Parameters.Add (new SqlParameter ("FullDay", strFullDay))
dc.Parameters.Add (new SqlParameter ("FirstHalfPaid", firstHalfPaid))
dc.Parameters.Add (new SqlParameter ("SecondHalfPaid", secondHalfPaid))
dc.Parameters.Add (new SqlParameter ("FullPaid", fullPaid))

Answer 2

Stinky - that SanitizeInput is almost certainly vulnerable to SQL injection.

I would use a parameterized SP (code generated from the DB schema, possibly tweaked by hand afterwards), with a .NET wrapper (also generated from the DB schema).

Answer 3

Direct rewrite of that in C# would be:

SqlCommand dc = new SqlCommand(String.Format(@"
    INSERT INTO Choices VALUES ('{0}', '{1}', '{2}', {3}, {4}, {5})
", SanitizeInput(strUser), SanitizeInput(strFirstHalfDay), SanitizeInput(strFullDay), SanitizeInput(Convert.ToInt32(firstHalfPaid).ToString()), SanitizeInput(Convert.ToInt32(secondHalfPaid).ToString()), SanitizeInput(Convert.ToInt32(fullPaid).ToString())), cn);

I use string.format a lot instead of string concatenation. It improves readability as well. I'm not familiar with VB, sorry.

I agree that using parameters is a better solution.

Answer 4

You could use a 3rd party data abstraction like Hibernate (on Java -- I don't know what's available for .NET).

Or you could write your own helper classes that allow syntax like:

Dim insHelper As New InsertionHelper(conn, "Choices");

insHelper.appendValue(strUser);
insHelper.appendValue(strFirstHalfDay);
insHelper.appendValue(strSecondHalfDay);
insHelper.appendValue(strFullDay);
insHelper.appendValue(firstHalfPaid); // overloaded for different data types
insHelper.appendValue(secondHalfPaid);
insHelper.appendValue(fullPaid);

insHelper.execute();

You could even add method chaining to the mix:

Dim insHelperAs New InsertionHelper(conn, "Choices");

insHelper
    .appendValue(strUser)
    .appendValue(strFirstHalfDay)
    .appendValue(strSecondHalfDay)
    .appendValue(strFullDay)
    .appendValue(firstHalfPaid)
    .appendValue(secondHalfPaid)
    .appendValue(fullPaid)
    .execute();

Answer 5

This approach is fine for a small project, however for anything of significant size you should consider either a 3rd party library for constructing your queries or developing your own.

The general pattern I use goes something like this: (I'm using Java syntax because that's what i am most used to.)

Query Template Properties File

some.query=INSERT INTO Choices VALUES ( '{0}', '{1}', '{2}', '{3}' )

Framework Code

public class SqlUtil {

    static Properties queries;
    static {
     queries = <LOAD QUERY PROPS>
    }

    public static SqlCommand createCommand(String propName, Object params ... ) 
     throws SqlException
    {
     String cmd = queries.getString(propName)

     if ( propName == null || "".equals(propName) )
      throw new SqlException("Query not found");

     int i=0;
     for ( Object param : params ) {
      cmd = cmd.replace("{" + i + "}", param)
      i++;
     }

     return new SqlCommand(cmd);
    }

}

Usage Code:

SqlCommand dc = SqlUtil.createCommand("some.query", SanitizeInput(strUser), SanitizeInput(strFirstHalfDay) )

Answer 6

I would consider this to be more readable and safer i.e. less prone to SQL Injection:

Dim dc As New SqlCommand("INSERT INTO Choices VALUES (@UserName, @FirstHalfDay, @SecondHalfDay, @FullDay, @FirstHalfPaid, @SecondHalfPaid, @FullPaid)", cn)

dc.Parameters.AddWithValue("@UserName", strUsername)
dc.Parameters.AddWithValue("@FirstHalfDay", strFirstHalfDay)
dc.Parameters.AddWithValue("@SecondHalfDay", strSecondHalfDay)
dc.Parameters.AddWithValue("@FullDay", strFullDay)
dc.Parameters.AddWithValue("@FirstHalfPaid", Convert.ToInt32(firstHalfPaid))
dc.Parameters.AddWithValue("@SecondHalfPaid", Convert.ToInt32(secondHalfPaid))
dc.Parameters.AddWithValue("@FullPaid", Convert.ToInt32(fullPaid))

When doing this for select statements you will get the added benefit if you are using SQL Server which will be able to cache the query plan as it will be the same string just paramterised. If you are not using SQL Server slight changes might be required. You also get another benefit for free which is slightly better performance through less string concatenation.

I would personally add the columns in after the Choices table name which would be less prone to breakage when adding new columns into the database table (assuming default values).

Answer 7

While I think your code is wonderful, I have a non-exhaustive list of potential improvements:

• You are writing SQL instead of using a mapping technology (NHibernate, Linq to SQL, EF, SubSonic, LLBLGenPro, etc.)
• Granted that you are writing SQL, you are not using parameters. At least you are correctly sanitizing the input as you do string-concatenation ... hopefully ....
• Granted that you are writing SQL without parameters, you are not using String.Format to do your string-building and string-formatting.
• Your variable names are messy. You are naming your variables with a bastardized Hungarian Notation convention. Hungarian Notation was originally intended to help your variable names convey how these variables are used, not to convey their types.
• You are coding to the implementation, not the interface. Your connection variable should be typed IDbConnection and your command variable should be typed IDbCommand. Not SqlConnection and SqlCommand. Use IDbConnection.CreateCommand().
• SanitizeInput should be renamed EscapeStringForSQL or something that indicates its actual intent. You don't need to escape numbers.

Answer 8

Make the SQL command a separate string before calling the constructor. It will be easier to read.

Instead of:

Dim dc As New SqlCommand("INSERT INTO SomeTable....",
                         Arg( blah blah blah ) )

Do this:

Dim sqlstr =
"INSERT INTO SomeTable...."

Dim dc As New SqlCommand(sqlstr, Arg(blah blah blah))

(Forgive my not knowing the correct VB syntax)

The thing is that pouring the literal string into the SqlCommand() constructor is just cramming a lot of stuff in one big command that someone has to break apart in his or her head later on.

Also, when you want to print out the SQL you are executing while debugging, it will be much easier to throw in a simple

Print "Now executing: " + sqlstr

Don't worry that you're creating an "extra" variable. The computer doesn't mind.