Design Pattern for Error Handling

Question

I've ran into this problem a few times on various projects, and I've wondered if there's a better solution than the one I normally end up using.

Say we have a series of methods that need to execute, and we want to know if something goes wrong within one of the methods and break out gracefully (potentially undo-ing any previous changes...), I typically do the following (pseudo C# because it's what I'm most familiar with):

private bool SomeMethod()
{
    bool success = true;
    string errorMessage = null;
    success = TestPartA(ref errorMessage);
    if (success)
    {
        success = TestPartB(ref errorMessage);
    }
    if (success)
    {
        success = TestPartC(ref errorMessage);
    }
    if (success)
    {
        success = TestPartD(ref errorMessage);
    }
        //... some further tests: display the error message somehow, then:
        return success;
}

private bool TestPartA(ref string errorMessage)
{
    // Do some testing...
    if (somethingBadHappens)
    {
       errorMessage = "The error that happens";
       return false;
    }
    return true;
}

I just wondered (and this is my question) if there's a better methodology for coping with this kind of thing. I seem to end up writing a lot of if statements for something that seems like it should be slicker.

I've been suggested having a loop over a set of delegate functions, but I'd be worried that would be over-engineering the solution, unless there's a clean way to do it.

Answer 1

I think you should probably be using exceptions. Note you should generally only be catching exceptions at the "top level" in your application.

private void TopLevelMethod()
{
    try
    {
        SomeMethod();
    }
    catch (Exception ex)
    {
        // Log/report exception/display to user etc.
    }
}

private void SomeMethod()
{
    TestPartA();
    TestPartB();
    TestPartC();
    TestPartD();
}

private void TestPartA()
{
    // Do some testing...
    try
    {
        if (somethingBadHappens)
        {
            throw new Exception("The error that happens");
        }
    }
    catch (Exception)
    {
        // Cleanup here. If no cleanup is possible, 
        // do not catch the exception here, i.e., 
        // try...catch would not be necessary in this method.

        // Re-throw the original exception.
        throw;
    }
}

private void TestPartB()
{
    // No need for try...catch because we can't do any cleanup for this method.
    if (somethingBadHappens)
    {
        throw new Exception("The error that happens");
    }
}

I have used the built-in System.Exception class in my example; you can create your own derived exception classes, or use the built-in ones derived from System.Exception.

Answer 2

You could perhaps try looking at the "Open/Closed" section of the SOLID Principle. In your example you could perhaps create an ITestRule interface which contains a method called CheckRule() that will updated your message and return a bool. You would then create an interface implementation for each rule you want to test, and add that class to a List<ITestRule> object. From the Redmondo example above, I would change to the following:

var discountRules =
                new List<ITestRule>
                    {
                        new TestPartA(),
                        new TestPartB(),
                        new TestPartC(),
                        new TestPartD(),
                    };

You would then pass the new List<ITestRule> to an evaluator which will loop through each of the classes and runs the CheckRule() method.

Answer 3

I try to stick to a principle known as 'Fail Fast'; methods should fail when they are supposed to, and return immediately with details of the error. The calling method then responds appropriately (re-throw the exception to its caller, log the details, show an error if it's a UI-bound method, etc): -

http://en.wikipedia.org/wiki/Fail-fast

However, this does not mean using exceptions to control the flow of your application. Just raising an exception when you could deal with it is generally bad practice: -

http://msdn.microsoft.com/en-us/library/dd264997.aspx

In your case, I'd re-write your code as (for example): -

private bool SomeMethod()
{
    bool success = false;

    try
    {
        TestPartA();
        TestPartB();
        TestPartC();
        TestPartD();

        success = true;
    }
    catch (Exception ex)
    {
        LogError(ex.Message);
    }

    //... some further tests: display the error message somehow, then:
    return success;
}

private void TestPartA()
{
    // Do some testing...
    if (somethingBadHappens)
    {
        throw new ApplicationException("The error that happens");
    }
}