I know there are a lot of programmers who do not want their application to crash out in the field and they are willing to do almost anything to stop that from happening - like swallow exceptions and let the program continue to run. But this is absolutely the wrong thing to do; it is much better for a program to crash than to continue running with unpredictable behavior and potential security vulnerabilities. - Jeffrey Richter (Framework Design Guidelines)

Bad code:

1 try 
2   { 
3     currentPageInContext = context.Session["CurrentPage"].ToString().ToLower();
4   }
5   catch { }

I don’t think most developers are aware of how expensive it is to raise exceptions, and if you develop so that exceptions are part of your programming logic like the above code, I think it’s time you made a change!

In the above example the developer is swallowing a potentional ArgumentNullReference to continue executing code… silly… this code could have been written like so…

1 currentPageInContext = context.Session[ "CurrentPage" ] as String;
2   if( null != currentPageInContext ) 
3   {
4     currentPageInContext.ToLower( CultureInfo.InvariantCulture );
5   }

So what’s happening here? The code is attempting to pull a value out of Session with a key of “CurrentPage”. It’s using the “as” keyword to cast to a type of String. Why use the “as” keyword? The reason for using the “as” keyword is because if the type cannot be cast to the type string it will return a value of “null” instead of raising an InvalidCastException. So the next step is to check that the returned value is in fact not null, if so then drop it to lower case. (Also, it’s a good habit to make use of the overload that allows you to specify the CultureInfo or IFormatProvider. If you don’t belive me, FxCop it!)

Another thought… instead of using…

1 DateTime.Parse( inputDate );

Use..

1 DateTime.TryParse( inputDate, out selectedDate );

Also, if you decide to catch an Exception and raise a more specific exception in it’s place make sure you toss in the old exception as the innerException parameter on your new Exception. Otherwise, you will lose the Stack Trace and other useful data for understanding what in the heck went wrong…

Here’s an example of what not to do:

1 try {
2     
3   }
4   catch (Exception e) {
5     throw new Exception(e.ToString());
6   }

The above example is bad for a couple of reasons. The first, well… catching an Exception then throwing another exception of the exact same type is expensive and a waste. If you’re not going to do anything with the exception then don’t catch it… If you really need to do it… do it like this…

This is better than above, but it’s still not optimal:

1 try {
2 
3  	}
4  	catch (Exception e) {
5  		throw;
6  	}

The following example is contrived, and probably still does not make sense… but if you have to re throw an exception from a catch block you might as well do it like this…

1 try {
2 
3   }
4   catch (Exception e) {
5     throw new ArgumentOutOfRangeException( "Dude... this message sucks!", e);
6   }
comments powered by Disqus