David Turvey's Blog

This is just an FYI, I’m sure most people know about this but I thought I’d blog about it because I’ve noticed a lot of stored procedures in the database at work that don’t take the schema into account when they drop the stored procedure.

When dropping some object from the database, e.g. a table or stored procedure, the following statement is often used:

IF EXISTS (
        SELECT *
        FROM sysobjects
        WHERE type = 'P' AND name = 'GetAlertGeographies' )
BEGIN
    DROP PROCEDURE [Members].[GetAlertGeographies]
END
GO

Now this might appear ok but it is not; the select statement that is checking the sysobjects table is not taking the schema into account. So this statement could attempt to drop the stored procedure even though it doesn’t exist because a stored procedure with the same name exists in a different schema.

This is the correct statement to use to take schemas into account:

IF EXISTS (
        SELECT TOP 1 NULL
        FROM information_schema.routines
        WHERE specific_name = 'GetAlertGeographies' AND specific_schema = 'Members' )
BEGIN
    DROP PROCEDURE [Members].[GetAlertGeographies]
END
GO

If you want to check for a table just change “routines” to “tables”.

As I’m sure everyone knows, you should always dispose of an object that you’ve created once you’re done using it. The most common way of doing this is using a try… finally block, where the object is disposed of in the finally block.
While there is absolutely nothing wrong with this, there is an easier way in C#; and that is to use a using block. Basically all the using block does is call Dispose on the IDisposable interface once the object referenced at the beginning of the using statement is out of scope.

So here’s how you would do it with a try… finally block:

bool isUnique = false;
SqlParameter[] parameters = new SqlParameter[1];
Utilities.Database database = new Utilities.Database();
SqlDataReader dataReader = null;

try
{
    parameters[0] = database.MakeInParam("@FieldName", SqlDbType.VarChar, 50, _name);
    dataReader = database.GetDataReader("Accounts.ExtraDetailsFieldExists", parameters);
    if (!dataReader.HasRows)
    {
        isUnique = true;
    }
}
finally
{
    if (dataReader != null)
    {
        dataReader.Close();
    }
    if (database != null)
    {
        database.Dispose();
    }
}

return isUnique;

And to do the same thing using the C# using block:

bool isUnique = false;

using (Utilities.Database database = new Utilities.Database())
{
    SqlParameter[] parameters = new SqlParameter[1];
    parameters[0] = database.MakeInParam("@FieldName", SqlDbType.VarChar, 50, _name);

    using (SqlDataReader dataReader = database.GetDataReader("Accounts.ExtraDetailsFieldExists", parameters))
    {
        if (!dataReader.HasRows)
        {
            isUnique = true;
        }
    }
}

return isUnique;

Isn’t that far more readable? Far less code and its very clear where the scope of each object ends. Now I hear you shouting what about closing the data reader! Luckily for us the base class of the SqlDataReader closes the data reader when Dispose is called, and the same applies to the SqlConnection class.

I thought I’d write a brief post on some exception handling guidelines after noticing in the code at work that there are a lot of try catch blocks that simply catch the base Exception class, among other bad exception handling practises.

A common mistake that a lot of people make is that they use try catch blocks to handle the flow of code, for example one might do the following:

try
{
    FileStream fileStream = File.Open(@"c:\file.txt", FileMode.Open);
}
catch
{
    MessageBox.Show("File does not exist!");
}

This is bad, don’t do it! You should rather be doing this:

if (File.Exists(@"c:\file.txt"))
{
    FileStream fileStream = File.Open(@"c:\file.txt", FileMode.Open);
}
else
{
    MessageBox.Show("File does not exist!");
}

Now of course you can put additional error handling around the File.Open statement above in case the file cannot be opened because there is a lock etc.

The next common bad practise is having an empty catch statement or catching the base Exception class. Having an empty catch statement (such as in the example above) is bad because there isn’t really much you can do in the catch block without the exception object, people often do this if they want to suppress an exception, which sometime you do want to do (more about that later). And why is catching the base Exception class bad? Well catching the base Exception class isn’t bad, catching only the base exception class show be avoided. If you’re only catching the base Exception class then you probably don’t have a good idea of what your code in the try block is doing, you should at least know some of the exception types that can be raised by your code, and if you don’t know you’re just being lazy! You simply need to hover over the method to see the exceptions that can be raised (assuming of course that the developer included the exceptions in the XML documentation).

So for example, we could modify our example above to the following:

if (File.Exists(@"c:\file.txt"))
{
    try
    {
        FileStream fileStream = File.Open(@"c:\file.txt", FileMode.Open);
    }
    catch (UnauthorizedAccessException)
    {
        MessageBox.Show("You do not have sufficient access rights");
    }
}
else
{
    MessageBox.Show("File does not exist!");
}

As I’m sure you know you can specify more than one catch block so you can handle certain exceptions differently. So the example above could be expanded as follows:

if (File.Exists(@"c:\file.txt"))
{
    try
    {
        FileStream fileStream = File.Open(@"c:\file.txt", FileMode.Open);
    }
    catch (UnauthorizedAccessException)
    {
        MessageBox.Show("You do not have sufficient access rights");
    }
    catch (IOException ex)
    {
        MessageBox.Show("Error reading file, reported error: " + ex.Message);
    }
    catch (Exception ex)
    {
        MessageBox.Show("Unknown error occured, reported error: " + ex.Message);
    }
}
else
{
    MessageBox.Show("File does not exist!");
}

You’ll notice than the exception base class is last, you should always order your exceptions from the most derived type to the least derived type.

Now I’m sure I’ve said more than once that catching the base Exception class is bad, but don’t take that to mean you should never do it, it simply means if you can catch a more specific type then you should. Situations often arise when you don’t know what the specific exception types are, for example when integrating with a third party component, they may not publish the possible exceptions, you could of course always use .NET Reflector to find out

When catching an exception be sure to always do something, i.e. handle the exception, this could be logging the error to a database, the event log, or simply displaying a message to the user. If you need to release resources if an exception occurs then use the finally block (or use a using block), don’t release the resources in the catch block!

This obviously isn’t an exhaustive list of the things not to do when handling exceptions, there are many websites out there with guidelines for exception handling best practises, so there is no excuse not to Google and learn!

CWE/SANS has released a list of what it terms the TOP 25 Most Dangerous Programming Errors. The list is quite interesting but most of it should be common sense even if you don’t always put it into practise, it also provides links on how to deal with the errors. I think this is a must read for all developers. Some notable errors which are very common at OB are included in the list, such as Improper Input Validation, Improper Encoding or Escaping of Output, Cleartext Transmission of Sensitive Information, Download of Code Without Integrity Check, etc etc.

http://www.sans.org/top25errors//?cat=top25