Remove Error Handling From Your Code

There are two main ways of directly dealing with error handling in modern programming: exceptions and error codes. Unfortunately, neither of them is any good. Although various techniques exists to aid in debugging - like asserting and logging - these methods are not really great for catching unexpected behaviour or the user being a douche. Essentially we are stuck with the two bad ones.

Exceptions

I hate exceptions. I mean -  I really really REALLY REALLY hate them. Much have been written about them, and while some loves them and some dislikes them, my job for this post is to make you all shy away from them in disgust … like the sane part of humanity does.

First, try to have a look in the Cambridge Dictionary.

exception

noun

Someone or something that is not included in a rule, group, or list or that does not behave in the expected way.

The keywords here are: “does not behave in the expected way”. This means that exceptions are for when your application does something unexpected. Something you couldn’t have foreseen. Now, if you place a progress bar on your Windows application, set the range to 0-100, and then set the progress to 101, an exception is thrown, and in the worst case, the entire application crashes. My angry questions are: Why is that an unexpected event? What happened to rigorously testing methods for any combination of invalid parameters, to ensure stable functionality? Is anybody in doubt about what the progress bar should do? It should obviously just go to 100 and stop there. What could possibly go wrong if it didn’t? It is so mind-blowingly stupid that it - almost - leaves me speechless. The real-life equivalent would be to have a manually dialed FM tuner, ranging from 88 MHz to 108 MHz, but if you turned the dial to 108.1 MHz, it would blow the main fuse.

- They cause infestation

The above example is one of my main gripes. Exceptions are infesting APIs, modules, and classes to an extent where even very basic usage potentially could be treated as an exception. If you update an existing API, it might introduce new exceptions. Were you aware of that? Are you handling them?

- They cause misuse

The infestation also results in gross misuse of exceptions, where the programmer has no other choice than to rely on them. For example, if you want to check if a file actually can be read, there is in many languages no other way of doing that, other than to rely on resource-expensive exceptions. This approach is stuffed down my throat by first depriving me of a way to check the state of a file, and secondly forcing me to use exceptions as control logic.

- They cause poor readability

Apart from exception handling effectively being a goto, and thus should be hated dearly, it also scatters the code all over the place. Especially if you handle more than one exception, it can quickly get pretty messy. In terms of readability, this is bad, and in the novel analogy, it corresponds to having to read paragraph four, in order to fully understand paragraph one. For the best possible readability, the error handling should be an integrated part of the program flow, and for that reason alone, exception handling is out.

- They cause lots of other problems

If you read posts about using exceptions, you will read about many other problems:

• Causing state inconsistencies.
• Causing complex and unclear code.
• Linus Torvalds hates them.
• Where and why exceptions are thrown is hard to read from code.
• They are often used for control flow.

Many answers to this are that you just have to implement them properly, and my answer to this is that if they are so hard to implement properly and potentially have all these bad side effects, maybe there is a better way to go about error handling.

- They are good for one thing though

Languages like Obj-C and Go, which rely heavily on error codes, still have exceptions, but in these languages, they are used as the name implies, an exception. If you have exceptions in Obj-C, your application is either in a state where it is about to crash or at least be severely hamstrung. Exceptions are exactly what they should be, reactions to events the programmer could not have foreseen.

Error Codes.

The other main approach to handling errors is using error codes. While better than exceptions, I am still not a big fan. The main reason is that it obfuscates the code. After each call, you will often have a long range of checks, and the code easily becomes a nested pyramid of a lot of basic checks.

This leads to code like this:

func DoSomething() 

{

  if result1, err := PerformOperation1(); err != nil 

  {

    log.Fatalf("Error occurred in operation 1: %s", err)

  } 

  else 

  {

    if result2, err := PerformOperation2(); err != nil 

    {

      log.Fatalf("Error occurred in operation 2: %s", err)

    } 

    else 

    {

      if result3, err := PerformOperation3(); err != nil 

      {

        log.Fatalf("Error occurred in operation 3: %s", err)

      } 

      else 

      {

        // YAY … Success  

      }

    }

  }

}

Even with the simplest error checking, this produces very ugly code and makes it much harder to find the important pieces. Readability suffers greatly. With extended error checking, it simply all just goes down the drain.

Secondly, checking for error codes can easily be omitted, which is even worse.

So how do we fix it?

Imagine we want to build a method that checks user credentials up against an SQL server. The method has to be able to return three access states:

• None.
• Limited.
• Full.

Limited access should be returned if an SQL request could not be completed, allowing the user to maybe edit the SQL setup. None and Full are returned if SQL is good, depending on whether user credentials checked out or not.

Using exceptions, the code would then look something like:

public enum RSAccessLevel

{

  None,

  Limited,

  Full

}

public RSAccessLevel CheckCredentials(string user, string password)

{

  string setup = "some_connection_string";

  string query = "SELECT * FROM table WHERE column = @Key";

  try

  {

    using (SqlConnection connection = new SqlConnection(setup))

    {

      SqlCommand command = new SqlCommand(query, connection);

      command.Parameters.AddWithValue("@Key", user);

      connection.Open();

      SqlDataReader reader = command.ExecuteReader();

      string storedPassword = "";

      if (reader.Read() == true)

      {

        storedPassword = reader.GetString(0);

      }

      reader.Close();

      if (password == storedPassword) return RSAccessLevel.Full;

      return RSAccessLevel.None;

    }

  }

  catch (SqlException)

  {

    return RSAccessLevel.Limited;

  }

  catch (Exception)

  {

    return RSAccessLevel.None;

  }

}

I would call this pretty normal code, which I have seen a ton of. While it by no means is bad code, it still has a lot of problems, and some of the more experienced guys might even spot several critical bugs.

One serious bug is that ExecuteReader does NOT cause an exception if the key is not found. This means that storedPassword will be left in the default state, and that any bogus userName with a blank password will get full access. Oops.

Also, there is no check for the password being null. 

Another flaw is the lack of using statements around the command and reader. In case of an exception, only the connection will be disposed immediately, and the command and reader will have to rely on garbage collection. This breaks the rule of disposing objects in reverse order of creation and can in some cases lead to side effects.

The code is, in general, much harder to understand than it ought to be. 

Because exceptions are used for “flow control” to return the Limited state, and because you need extensive knowledge about how ExecuteReader works, a serious bug was introduced. 

Rant: Why does reading a missing key in Dictionary cause an exception, but reading a missing key in SQL does not? That is simply just inconsistent. Microsoft, are you listening, or are you busy writing planet-entry code for a Mars lander?

So what is a better solution?

The first rule about error handling is, you don’t talk about error handling.

public RSAccessLevel CheckCredentials(string user, string password)

{

  // Data needed for the method

  // TODO: fix the damn hard-coding

  string setup = "some_connection_string";

  string query = "SELECT * FROM table WHERE column = @Key";

  // create required objects

  RSSqlConnection connection = new RSSqlConnection(setup);

  RSSqlCommand command = new RSSqlCommand(connection, query);

  

  // fetch the data stored for the user

  command.SelectColumn(“@Key”);

  string storedPassword = command.ReadDataAsString(user);

  // determine access level

  RSAccessLevel result = RSAccessLevel.None;

  if (command.Valid == false)

  {

    result = RSAccessLevel.Limited;

  }

  else if ((password != null) && (password == storedPassword))

  {

    result = RSAccessLevel.Full;

  }

  // clean up

  command.Dispose();

  connection.Dispose();

  // done

  return result;

}

Obviously, both examples should get rid of the horrible hard coding, but I kept it for comparison, also, I did not test the SQL functionality.

In the code above, error handling has simply been removed because it is not important in this context. All that matters is to determine the access level. As I mentioned in the post about wrappers, I obviously wrapped the SQL functionality and made the constructors and methods exception-safe, meaning that is where the error handling is. The wrappers are thin wrappers, adding no functionality other than exception safety, except for RSSqlCommand where I added a bit of syntactic sugar (just because I could :)), and updates the _valid flag for each read.

The code is now clean and pretty straightforward. Functionality has been split into sections, and few should be in doubt about what is going on here. Also, if any exceptions are thrown here, they are truly unexpected and not something used for control flow.

The comments here is not strictly required, but they improve readability, and even if the code changes significantly, they will not get obsolete.

A note on the clean-up section. Normally the garbage collection will take care of disposing of any objects out of scope, but especially with databases, complex connections, or interfaces, the creation and disposal order could be important. As a rule of thumb, always dispose of in reverse order of creation. In for example C#, you could secure this by wrapping the constructors in a “using” statement, but when it is important, I like to do it manually, thus signaling to the reader: “I know what I am doing… ish”.

As for the wrappers, they are pretty straightforward and do nothing but wrap the SQL functionality and a _valid flag. If you, for some reason, needed extended error handling, here is where you would do it. In general, do any error handling as early as possible.

I also added the _column property to make the ReadDataAsString a bit cleaner, but that is just personal preference. How you do it is entirely up to your coding preferences and your needs.

// RSSqlCommand

public class RSSqlCommand : RSDisposable

{

  public bool Valid { get { return _valid; } }

  private SqlCommand _command;

  private bool _valid;

  private string _column;

  public RSSqlCommand(RSSqlConnection connection, string query)

  {

    try

    {

      _command = new SqlCommand(query, connection.Connection);

      _valid = true;

    }

    catch

    {

      _valid = false;

    }

  }

  public void SelectColumn(string column)

  {

    _column = column;

  }

  public string ReadDataAsString(string key)

  {

    string result;

    try

    {

      _command.Parameters.Clear();

      _command.Parameters.AddWithValue(_column, key);

      SqlDataReader reader = _command.ExecuteReader();

      result = reader.GetString(0);

      _valid = true;

    }

    catch

    {

      result = “”;

      _valid = false;

    }

    return result;

  }

}

// RSSqlConnection

public class RSSqlConnection : RSDisposable

{

  public SqlConnection Connection { get; }

  public RSSqlConnection(string connectionString)

  {

    try

    {

      Connection = new SqlConnection(connectionString);

      Connection.Open();

    }

    catch

    {

    }

  }

}

If you read about The Lego Principle, it is clear that RSSqlCommand is on top in the hierarchy. It uses data from RSSqlConnection, and would be allowed to call methods. The other way around, though, is prohibited, and RSSqlConnection must remain blissfully unaware of RSSqlCommand.

Takeaway

Error handling should be removed from code providing application functionality and placed in classes and wrappers at lower levels. The lower the level, the better. In general, class methods and data should always return a predictable result, no matter the input. There are several techniques for this, which I will cover in future posts, but it is far from impossible to build large and complex applications where error handling is only done at the very lowest levels.

/Lars