The art of creating exceptions

This will be my final piece on exception handling. It’s been fun, but the show must go on, right? Got any suggestion on what I should rant about in this blog? Leave a comment.





Please take a moment and think about WHY you throw exceptions.

You done? Have you come up with a reason? You are probably thinking: “That b!@tch is a total moron. I throw exceptions to know that something exceptional have happened!”. If that is the case, please stop reading here. (The first part of the thought is totally fine by me, I can be a total moron.)

.
.
.
.
.
.

I SAID STOP READING!!!!

.
.
.
.
.
.
.

Really!

.
.
.
.
.
.
.
.
.

No? Well. Ok then. Want to be an exception master? Got some ninja skills? Have you bought a jump suit? I have actually created a diploma (yes, It’s true) that I will email to anyone who leave a comment that says: “I throw exceptions to be able to try to prevent them in the future”. Because that’s what you should really try to do. Sure, some exceptions that are thrown CAN’T be prevented. But a lot of them can.

Let’s take FileNotFoundException as an example. It can in MOST cases be prevented, if you are not a lazy SOB, by checking if the file exists before trying to open it. The exception can STILL be thrown if the file is deleted between the check and and you opening it. Life is harsh, isn’t it.

The problem is that if you do not provide any contextual information you will have a hard trying to prevent it in the future.

Instead of just writing

  throw new FileNotFoundException("Ohhh, the file was missing! *buhu*");

do write

  throw new FileNotFoundException(string.Format("Failed to find '{0}'.", amazingFileName));

Even better would have been if you could add some more context and add the filename as a parameter:

  throw new FileNotFoundException(string.Format("User photo for user #{0} was not found", userId)) { Filename = amazingFilename };

That gives a lot more information to work with, don’t you think? When designing exceptions you should ask yourself “What kind of context information can I provide in this exception to make it easier to prevent it?”. Regarding a file it’s quite obvious. Hence I would design the exception like this:

  public class FileNotFoundException : IOException
  {
      public FileNotFoundException(string message, string fileName)
	     : base(message)
	   {
			Filename = fileName;
	   }

       // *ALWAYS* include a constructor which takes a inner exception.
	   //
	   // I WILL HUNT YOU DOWN AND GIVE YOU SOME SPANKING IF YOU DON'T!
       public FileNotFoundException(string message, string fileName, Exception inner)
		: base(message, inner)
	   {
	   }

      public string Filename { get; private set; }
  }

Let’s look at another example.

I’ve talked about a DataSourceException in previous posts. It could be used to report problems in the data layer (regardless of the type of data source). The first thing that comes into my mind is that we have a target and some parameters. When using a web service, the target is the URI and the parameters are anything posted. Likewise, for SQL the target is a SQL query and the parameters is the parameters used in the SQL query. Doing it like this creates a potential security risk. Do NOT expose exception details for the end users, never ever.

  public class DataSourceException : Exception
  {
	public DataSourceException(string message, string target, Dictionary<string, object> parameters)
	     : base(message)
	{
			Filename = fileName;
	}

       public FileNotFoundException(string message, Exception inner, string target, Dictionary<string, object> parameters)
		: base(message, inner)
	{
	}

	public string Target { get; private set; }
	public Dictionary<string, object> Parameters { get; private set; }

  }

How it can be used for a webservice request:

	try
	{
		var user = myWebSvc.GetUser(userId);
		if (user == null)
		  throw new InvalidOperationException(string.Format("Failed to find user #{0}", userId)); //kilroy was here
	}
	catch (WebException err)
	{
	    throw new DataSourceException(string.Format("Failed to download user.", userId), myWebSvc.Uri, userId);
	}

Or for a database:

	string errMsg = "Failed to update user";
    ExecuteCommand(cmd => {
		cmd.CommandText = "UPDATE user SET name=@name WHERE id=@id";
		cmd.AddParameter("name", name);
		cmd.AddParameter("id", userId);
	}, errMsg);

Where did all code go? It’s a small method looking like this:

	public void ExecuteCommand(Action<IDbCommand> action, string errMsg)
	{
		// CreateConnection() uses DbProviderFactory and
		// throws DataSourceException if connection fails
		using (var connection = CreateConnection())
		{
			var cmd = connection.CreateCommand();
			try
			{
				action(cmd);
				cmd.ExecuteNoQuery();
			}
			catch (DbException err) //notice that I only handle DbException?
			{
				// WTF is this? Check next code snippet.
				// note that I *return* an exception from the extension method (instead of throwing it)
				throw cmd.ToException(errMsg, err);
			}
			finally
			{
				cmd.Dispose();
			}
		}
	}

I used a small extension method:

    public static class DataExtensions
	{
		public static Exception ToException(this IDbCommand command, string errMsg, Exception inner)
		{
			var parameters = new Dictionary<string, object>();
			foreach (var p in command.Parameters)
				parameters.Add(p.ParameterName, p.Value);
			throw new DataSourceException(errMsg, inner, command.CommandText, parameters);
		}
	}

Summary

You should be fine if you stop to think “I throw exceptions to inform that something exceptional happens” and instead start thinking “I throw exception to help try to prevent the exceptional cases from happening in the future”. Having that mindset helps you create much more detailed exception classes (and hopefully also provide that information).

Each time you are about to throw an exception ask yourself: What information can I provide in this method to make it easy to find out why the exception was thrown? It might take a couple of minutes longer, but how long does it take to debug your application if you do NOT get that information? Usually a lot longer.

Action points for you:

  1. Create exception classes containing as much context information as possible
  2. Always create a constructor that takes an inner exception
  3. Throw exceptions to help prevent them in the future
  4. Try to include as much context information as possible

Update

Hold the presses. I got so carried away by my bright ideas for this article that I got a bit blinded by the brightness. The most important reason to a throw exception is to be able to handle it. That’s why you throw DataSourceException and not Exception. The more specific, the merrier.

InvalidOperationException: “Exception” in disguise? Or just the new ApplicationException?

Microsoft added ApplicationException with the idea that all exceptions that you made should derive it. Fair point, your should be able to handle your own exceptions, right? The problem is what nobody used it and therefore Microsoft changed it’s recommendation to NOT use it. It’s just lying there in the framework like an old aunt that just don’t want to go away (I still love you all my dear aunts).

InvalidOperationException is also an exception that should be blown to hell. The description of the exception says this:

The exception that is thrown when a method call is invalid for the object’s current state.

What does that mean in the real world? When do exceptions get thrown in general? Well. When something unexpected have happened, as in when a method is called when it isn’t supposed to be. How else can an exception be raised? When not calling a method?

The InvalidOperationException is really like an Exception in a disguise. Try to search in a couple of open source projects after it (or your own). It’s thrown a lot. It’s a poor mans choice to exception handling. Ohhh, I’ve been guilty one too many times too. It’s so handy since it can be applied to all exceptional cases.

The reason to why I wrote this post is that I was debugging an ASP.Net MVC application and found that MS throws it when an action cannot be found in a controller. The result is that a Internal Server Error page is shown instead of a File Not Found page. Sure, it’s an error in the application and the proper controller have been found. But do the browser care about which framework is used in the web server? No. File not found is therefore the proper status code.

Microsoft should have created an ActionNotFound exception instead and derived it from HttpException (and set the 404 status code in the constructor initialization).

Edit:
I’ve filed a bug report for ASP.Net MVC: http://aspnet.codeplex.com/workitem/7588

Why autogenerated mstests is bad for you!

ms can generate complete test classes for you that test and access all methods and properties, no matter if they are private or public. Isn’t that awesome? Well no.

Let me do a statement first: I’m not fond of auto-generated code, no matter what it is for.

Why? Because writing code makes you think. You’ll have a better chance to find design error earlier than if you get a lot of stuff generated for you. You also need to know how stuff works. If everything is auto-generated for you, you don’t have to care. Right?

Back to mstests.

Reason 1:
The access to private/protected members/properties is bad for you. If you can’t manage to test those methods/properties through the public API it’s most likely that they can’t be called from your regular application either. Either remove the code that you can’t test or refactor the class.

Reason 2:
The  generated test classes includes one test per method. Inexperienced unit testers might think that it’s enough with one test per method: They either just write one test or includes all the different tests in the same test method. Both of those options make it a lot harder to test and get robust applications.

Reason 3:
Readability.The generated tests is harder to read:

(i’ll post samples here later)

Conclusion: Build all your tests by yourself (it makes you think about your design once more). Remove code that you can’t test through the public interface (or refactor).

Do NOT catch that exception!

Ohhh, I’ve recently seen one to many application where the developers try to be safe by catching exceptions all over the place. It can be everything from a small simple method that doesn’t throw that many exceptions to a large method with multiple try/catch statements. Most of those catch blocks just logs the exception and rethrow it.

This post discusses how those problems can be solved.




Ohhh, I’ve recently seen one to many application where the developer try to be safe by catching exceptions all over the place. It can be everything from a small simple method that doesn’t throw that many exceptions to a large method with multiple try/catch statements. Most of those catch blocks just logs the exception and rethrow it.

Please do not catch that exception. Let it travel down the call stack. I promise you that you won’t regret it. You’ll get a warm and fuzzy feeling next time you’ll test your application. I promise you! Really!

Why? You’ll get a complete exception with all the details you need to fix the error, it’s very easy to loose information if you catch/wrap/rethrow exceptions . The problem with all of those try/catch blocks is they doesn’t help your application in any way. They just clutter up the code and your log files.

If you still want to catch exceptions: I’ll try to explain the way to do it.

Catching exceptions in the correct way.

Yeah. Proper exception handling can be something wonderful. You’ll smile every time you get an exception. First of all: Exceptions only happens when something exceptional occurs.  Please have that in mind, because it helps you remember that you don’t need those try/catch block everywhere.

That brings us to the golden rule about when to catch exceptions:

Only catch exceptions that you can handle to rescue the situation.

That means that you should only catch exceptions if you, by handling it, can let the application continue as (almost) expected. Let me rephrase that: Only catch a exception if you, by catching it, can let the method return a result as promised.

There are of course exceptions, namely two of them:

Do not let layer specific exceptions propagate up the call stack.

For instance, if the data layer fails and throws a SqlException (if you are using Sql Server) you should catch it and wrap it inside a more generic exception. Create a DataSourceException which could be used even if you switch data source from a database to a Web Service. You should of course add details to the exception such as the method name, parameters or anything else that can be useful. Do NOT forget to add the original exception as  inner exception.

public class UserRepository : IUserRepository
{
    public IList<User> Search(string value)
    {
        try
        {
              return CreateConnectionAndACommandAndReturnAList("WHERE value=@value", Parameter.New("value", value));
        }
        catch (SqlException err)
        {
             var msg = String.Format("Ohh no!  Failed to search after users with '{0}' as search string", value);
             throw new DataSourceException(msg, err);
        }
    }
}

Update 2013-03-01: I’m not following this rule any more. The only time I wrap exceptions is if I add more context information to help me prevent the exception in the future. It doesn’t really matter if the data layer exception is passed up the call stack, since it won’t be handled but just logged in the top layer

If you don’t want to display data layer specific information to the user, then simply display something like "An unexpected error occurred" instead of showing exception information. The exception information doesn’t really help your user either way.

Try/Catch all is OK in the layer closest to the user.

Ok, so you have let the exceptions propagate through all your layers to the presentation layer (winforms/asp.net etc). Fine. Go ahead, give the user some fancy error pages. Also log those errors. But just don’t log them. Notify a support team. Send an email to the project lead.

When everything else fails

Are you not sure that your new exception policy will catch all of those exceptions? Is it scary? The I got the solution for YOU!

Asp.Net Implement Aplication_OnError(object source, EventArgs e) in your global.asa. It will be called for all unhandled exceptions.
WinForms Subscribe on Application.ThreadException
WinServices Subscribe on AppDomain.CurrentDomain.UnhandledException. The problem with this event is that it will still terminate the application. But there’s a way around that too.
WCF WCF: Implement IErrorHandler and log the exceptions in it (added 2013-03-01).
ASMX ASMX: Create a custom Soap Extension. (added 2013-03-01).

In the next blog post I’ll discuss common mistakes in those catch blocks and how your tailor made exceptions should look like.

Update 2013-04-17

I’ve written a new exception series. Starting with What are exceptions?

Simplified autofac registrations

I’m building an application which will monitor other applications (memory using, start them if they crash etc). I’m trying to stop re-inventing the wheel (ohhh it’s fun to do everything yourself) and start using other components. This time I needed a IoC container and autofac seemed like a good match for me.

To simplify IoC registration I created an attribute which I use on all my components. In this way I don’t have to remember to register each component, it’s done by the help of the attribute.

Using the attribute without constructor parameters will register the component with all interfaces.
[sourcecode language=”csharp”]
[Component]
class MyComponent : IConsumer<MonitorEvent>, ISomeService
{
//[….]
}
[/sourcecode]

While using the constructor will only register one interface.
[sourcecode language=”csharp”]
[Component(typeof(ISomeService)]
class MyComponent : IConsumer<MonitorEvent>, ISomeService
{
//[….]
}
[/sourcecode]

The autofac registration looks like this:
[sourcecode language=”csharp”]
internal class Program
{
public static IContainer Components { get; private set; }

private static void Main(string[] args)
{
var builder = new ContainerBuilder();

//i = interface type, c = concrete type
Assembly[] assemblies = AppDomain.CurrentDomain.GetAssemblies();
assemblies.Each(assembly => assembly.FindComponents((i, c) => builder.RegisterType(c).As(i).SingleInstance()));

Components = builder.Build();
}
}
[/sourcecode]

Quite smooth, huh?

Extension methods making it possible:
[sourcecode language=”csharp”]
public static class ComponentFinder
{
public static void Each<T>(this IEnumerable<T> enumerable, Action<T> action)
{
foreach (T item in enumerable)
action(item);
}

public static void FindComponents(this Assembly assembly, Action<Type, Type> action)
{
Type componentAttribute = typeof (ComponentAttribute);
foreach (Type type in assembly.GetTypes())
{
object[] attributes = type.GetCustomAttributes(componentAttribute, false);
if (attributes.Length == 0)
{
type.GetInterfaces().Each(i => action(i, type));
continue;
}

foreach (object attribute in attributes)
{
Type interfaceType = ((ComponentAttribute) attribute).InterfaceType ?? type;
action(interfaceType, type);
}
}
}
}
[/sourcecode]

Getting components is done by the resolve method:
[sourcecode language=”csharp”]
Program.Components.Resolve<IMyService>.SendMessage("Hello world");
[/sourcecode]

TodoApp – Part 3: Fix that data source.

Let’s continue with the refactoring. What are the main window really doing?? Well.

  1. Displaying todo list
  2. Loading and saving stuff to the database

That’s one thing too much. We’ll implement a Repository. imho, Fowler is wrong in his explanation. The most important goal with the Repository pattern is the following line: “Repository also supports the objective of achieving a clean separation and one-way dependency between the domain and data mapping layers”. That’s what the pattern is all about, period. We do not want our domain to have any knowledge about the data source.

In later versions, the data source will be a REST web service publishing stuff as JSON. By implementing a Repository now, we get the bonus that we do not have to do that many modifications in later versions. All we need to do is to switch implementation class from one to another (since we are using an interface and not a class in our code).

The new class is called ItemRepository. The application is a bit more readable now than the first version, right?

TodoApp – Part 2: Single responsibility is what we need.

In our journey towards hardcore we need to refactor the shitcore into a bit more softcore =) I’m not going to show all source code in this blog entry. You can look at the code here.

It will take some iterations before the code is quite usable. In this iteration we will focus giving each method a single responsibility, which means that each method should only contain the logic for one thing. The constructor is a kick ass example of that. Lets look at it.

First of all, it’s responsible of connecting to the database.

[sourcecode language=”csharp”]
public MainWindow()
{
InitializeComponent();
_db = new SQLiteConnection("Data Source=todomachine.db;Version=3;DateTimeFormat=Ticks;");
_db.Open();
[/sourcecode]

then, it’s also verifying that the table exists in the database.

[sourcecode language=”csharp” firstline=”6″]
//create table if it do not exist.
if (!DoesTableExist(_db, "todo_items"))
{
SQLiteCommand command = new SQLiteCommand(_db);
command.CommandText =
@"CREATE TABLE todo_items(
id INTEGER PRIMARY KEY AUTOINCREMENT,
title VARCHAR(40) NOT NULL,
description text NOT NULL
);";
command.ExecuteNonQuery();
}
[/sourcecode]

and finally it’s loading stuff into the list box.
[sourcecode language=”csharp” firstline=”18″]

// load all todo items.
SQLiteCommand mycmd = new SQLiteCommand(_db);
mycmd.CommandText = "SELECT * FROM todo_items";
var reader = mycmd.ExecuteReader();
while (reader.Read())
{
lbTodos.Items.Add(new Item { Id = (long)reader["id"], Title = (string)reader["title"] });
}
}
[/sourcecode]

Let’s fix that and write a little bit of documentation for each method.

btnNew_Click

Next we have btnNew_Click which is used to save a new item:

[sourcecode language=”csharp”]
private void btnNew_Click(object sender, EventArgs e)
{
var frm = new TodoItemForm();
frm.ShowDialog(this);

var cmd =
new SQLiteCommand("INSERT INTO todo_items (title, description) values(@title, @description)", _db);
cmd.Parameters.Add(new SQLiteParameter("title", frm.tbTitle.Text));
cmd.Parameters.Add(new SQLiteParameter("description", frm.tbDescription.Text));
cmd.ExecuteNonQuery();

lbTodos.Items.Clear();
// load all todo items.
SQLiteCommand mycmd = new SQLiteCommand(_db);
mycmd.CommandText = "SELECT * FROM todo_items";
var reader = mycmd.ExecuteReader();
while (reader.Read())
{
lbTodos.Items.Add(new Item { Id = (long)reader["id"], Title = (string)reader["title"] });
}
}
[/sourcecode]

It has two responsibilities: Saving an item into the database and refreshing the listbox.
Fortunately, we have already created a method for the listbox in the constructor refactoring. Let’s just add that the listbox should be emptied before adding all items.

lbTodos_MouseDoubleClick

lbTodos_MouseDoubleClick is used to edit an item.

[sourcecode language=”csharp”]
private void lbTodos_MouseDoubleClick(object sender, MouseEventArgs e)
{
Item item = (Item)lbTodos.SelectedItem;

SQLiteCommand mycmd = new SQLiteCommand(_db);
mycmd.CommandText = "SELECT * FROM todo_items WHERE id=" + item.Id;
var reader = mycmd.ExecuteReader();
reader.Read();

var frm = new TodoItemForm();
frm.tbDescription.Text = (string)reader["description"];
frm.tbTitle.Text = (string) reader["title"];
frm.ShowDialog(this);

var cmd =
new SQLiteCommand("UPDATE todo_items set title=@name, description=@description WHERE id = @id", _db);
cmd.Parameters.Add(new SQLiteParameter("name", frm.tbTitle.Text));
cmd.Parameters.Add(new SQLiteParameter("description", frm.tbDescription.Text));
cmd.Parameters.Add(new SQLiteParameter("id", item.Id));
cmd.ExecuteNonQuery();

lbTodos.Items.Clear();
// load all todo items.
SQLiteCommand cmd3 = new SQLiteCommand(_db);
cmd3.CommandText = "SELECT * FROM todo_items";
reader = cmd3.ExecuteReader();
while (reader.Read())
{
lbTodos.Items.Add(new Item { Id = (long)reader["id"], Title = (string)reader["title"] });
}

}
[/sourcecode]

Responsibilities:

  1. Load item from database
  2. Populate item form
  3. Save stuff from the item form into the database
  4. Populate list box

Here we’ll refactor the item form to use the Item class, and we’ll therefore make the item class standalone (instead of being a private class inside MainWindow). That should take care of #2.

#4 have already been fixed thanks to the previous method refactorings.

We’ll move the item loading into a separate method.

Done

The first refactoring is now done. The goal of this post was to help you understand what Single responsibility really means, although we are not quite done with the guideline yet. The MainWindow form still have many different responsibilities. But first let’s explore exception handling in the next post.

TodoApp – part 1: From shitcore to hardcore

The easiest way to learn how to improve ones coding is by examples. I’ve therefore created an example application which is really crappy written application. It’s a todo app where you can enter your todos (but not delete them ;)). The idea is to take this basic winform application and turn it into a client/server app utilizing different open source libraries for logging, IoC and web services.

The sourcecode is available at codeplex, each check in will represent a blog post. (Check each diff to see what I’ve refactored each time).

Read the source code and check the next blog entry for the first refactor iteration.

DRY KISS, SR!

Explains what “DRY KISS, SR” is all about.




omg! wtf?! Nahh, this ain’t a porn blog. The title is three coding principles that I try to follow when I code. And this blog is going to be all about them.

You’ll find code refactorings, insights and such that I discover during my days as a software architect/developer.

As you might notice, no entries will be elaborate or poetic. They will be short and concise: HARDCORE!

DRY – Don’t repeat yourself.

Most of you have probably heard of this principle. You can read more about it at wikipedia.

Code duplication is evil since you:

  1. can get the same bug in a lot of different places
  2. refactoring becomes hard (try finding all the places you have the duplicate code in)
  3. larger programs -> more code to maintain
  4. your application design is most likely bad as a rotten apple

Let’s focus on the last entry. Sorry, but it’s true. If you need to duplicate code, you have done something wrong. Go and take a coffee. Go through your code and try to refactor it. Ask a colleague of yours. DO NOT CONTINUE UNTIL YOU SOLVE THE PROBLEM. Trust me. You’ll regret it later.

KISS – Keep it simple (and) stupid

I have a really active brain. It comes up with all sorts of cool stuff and solutions when programming. That’s a BIG problem when coding. Coding is not about having the coolest solution or the most fancy features. It’s about doing as little as possible to get the job done. I do not mean that you should take shortcuts or writing shit code. I’m talking about doing a simple solid solution which get’s the job done but nothing more.

Time estimation is still one of the hardest thing to do when developing software. How long have we been coding? 40 years? And we still can’t do proper time estimations. El oh el! Do you know why? Because most programmers do not keep it simple! We love to learn new stuff and do cool solutions.

Finish the application first! Then do the fancy stuff.

Read more about KISS at wikipedia.

SR – Single responsibility

Have you ever seen a cop (Mr. Police) mow a lawn when on active duty? (If you have, please give him this link). I thought not. Why? Because doing a lot of different things would make him a bad cop. It’s the same with your classes, methods and projects. Don’t let a class both fetch the users from the database and present the info to the user. Do not let a method both send an email and eject the CD tray.

If a class is called UserReporsitory it should not do anything else than fetch/save you users from/to your data source. That’s what single responsibility means.

Why?! you ask. Well, letting your method/class/project doing a lot of stuff will make it hard to understand what your application does. It will also make it harder to understand. But the most important thing is: It will make it hard to test your application. (unit tests). And you don’t want that. Unit tests are a life saver when your application grows. All those green dots that says that the tests have passed will give you a warm fuzzy feeling inside when your application is 5M lines long. Imagine how hard it would be to test an app that large when all your classes do a lot of different stuff. Imaging a cop mow a lawn.

Read more about single responsibility @ objectmentor || oodesign

That’s it

Again. Welcome to my blog. Leave a comment or two.