Bad code examples (part 2)

I just came across some really bad code and I just had to post it right now. Some of the mistakes have been covered in my previous post, but what-the-heck! Here is the offending code:

public static NameValueCollection getGlobalVariables(string keyPrefix) {

    string strConnection = Magic.GetConnectionString();

    SqlConnection sqlConnection = null;

    NameValueCollection nvList = new NameValueCollection();

    try {

        sqlConnection = new SqlConnection(strConnection);

        SqlCommand sqlCommand =

                new SqlCommand("SELECT [something] FROM [somewhere] WHERE Name LIKE @Name"", sqlConnection);

        sqlCommand.Parameters.AddWithValue("@Name", keyPrefix + "%");

        sqlConnection.Open();

        SqlDataReader myReader = sqlCommand.ExecuteReader();


        while ( myReader.Read() )

            nvList.Add(myReader.GetString(0), myReader.GetString(1));


        myReader.Close();

    }

    catch ( Exception e ) {

        nvList = null;

        throw e;

    }

    finally {

        if ( sqlConnection != null )

            sqlConnection.Close();

    }

    return nvList;

}

Please note that the connection string is coming from a more or less sensible place, removed to protect the innocent…

So what’s wrong with this code? Lots!

• Using SqlConnection and not DbConnection is evil
• Not applying the "using" pattern for connections is evil
• try-catch-finally is evil
• Catching "Exception" is evil
• "throw e" is evil

Just the about the only two good things about this code is the fact that it is static and it’s using parameters and not concatenated strings for the sql command. So, how would the code look if I had my way with it? Something like this I guess:

public static NameValueCollection GetGlobalVariables(string keyPrefix) {

        using ( DbConnection sqlConnection = ConnectionFactory.DefaultConnection ) {

        NameValueCollection nvList = new NameValueCollection();

        DbCommand sqlCommand = sqlConnection.CreateCommand();

        sqlCommand.CommandText = "SELECT [something] FROM [somewhere] WHERE Name LIKE @Name";

        DbParameter sqlParam = ParameterFactory.CreateInputParameter("Name", keyPrefix + "%");

        sqlCommand.Parameters.Add(sqlParam);

        sqlConnection.Open();


        using ( DbDataReader reader = sqlCommand.ExecuteReader() ) {

            while ( reader.Read() )

                nvList.Add(reader.GetString(0), reader.GetString(1));

        }

        return nvList;

    }

}

That’s my 2 cents anyway… I’m sure that are other optimizations that can be done as well, please post any suggestions in the comments section.