Using extension methods in the decorator pattern

Question 1

After reading an exceptional article here about the decorator pattern applied to functions, I saw an opportunity to reduce a lot of boilerplate code in a console application project I’ve been working lately.

The app gathers information about the workstation it is run from, and saves that data to a central database. So, I have a lot of string methods, and most of them can throw an exception. Currently, a lot of them look like this:

public string GetDescription()
{
    try
    {
        Registry.GetValue("HKEY_LOCAL_MACHINE\SYSTEM\CurrentControlSet\services\LanmanServer\Parameters", "srvcomment", "").ToString();
    } 
    catch(Exception ex)
    {
        return "ERROR" + ex.ToString()
    }
}

NOTE: There are a lot of methods just like this one.

So I ended up with this extension method:

public static GetValueOrException(this Func<string> method)
{
    try{ return method(); }
    catch(Exception ex){ return "Error: " + ex.ToString(); }
}

this allows me to get rid of the try…catch clauses in every method (there are 15+ of them)

Now, I have two options to consume my new extension method:

Change my string methods to instead return Func<string> like so:

public Func<string> GetDescription()
{
    return ()=> { return Registry.GetValue("HKEY_LOCAL_MACHINE\SYSTEM\CurrentControlSet\services\LanmanServer\Parameters", "srvcomment", "").ToString(); }; 
}

This might look awkward but it does allow for chained invocation (Linq style – I love it) E.G.:

bar.GetDescription().ValueOrException();

Leave my string method as is, and cast to Func<string> every time I need the extension method
```
((Func<string>)bar.GetDescription).ValueOrException();
```

Option 1 feels more natural to use, however option 2 feels more natural to write. Which option is best, and why do you think so? I should mention that it is very unlikely for another people besides myself to also work on this code.

Question 2

public static GetValueOrException(this Func<string> method)
{
    try{ return method(); }
    catch(Exception ex){ return "Error: " + ex.ToString(); }
}
public Func<string> GetDescription()
{
    return ()=> { return Registry.GetValue("HKEY_LOCAL_MACHINE\SYSTEM\CurrentControlSet\services\LanmanServer\Parameters", "srvcomment", "").ToString(); }; 
}
bar.GetDescription().ValueOrException();

Why is this a horrible idea?

Because you just successfully eliminated the function the exceptions had originally: Error handling.

The premise when using LINQ like notations, is that you have a valid set of data in a fixed formate at each step of execution.

By intercepting the exception too early, and mangling it into the same format as a valid output, you are breaking with that premise. Instead of the valid input, you are now passing the error message down to subsequent operations.

With no way to distinguish the error case from a valid result.

But this mistake isn’t just new to your addition – it was already present on your original code:

public string GetDescription()
{
    try
    {
        Registry.GetValue("HKEY_LOCAL_MACHINE\SYSTEM\CurrentControlSet\services\LanmanServer\Parameters", "srvcomment", "").ToString();
    } 
    catch(Exception ex)
    {
        return "ERROR" + ex.ToString()
    }
}

That return "ERROR" + ex.ToString() is a problem. Because that is not how to properly handle an error case. You literally can’t distinguish at all whether this supposed error message was generated by this function, or if it was actually read from the registry.

Whenever you encounter an Exception, there are only 3 valid approaches:

Swallow the exception, and continue with a fail safe default.
Capture and encapsulate the exception in a more generic exception.
Translate the exception into a different, dedicated and distinguishable channel for error reporting. (E.g. encoding function success status in an enum return value, while the actual results are returned by reference.)
(optionally) Report the error to a logging component, before continuing with one of the 3 options above.

What you did was neither. You mixed the error state into the run time state of your application, tainting it and making the behavior of all subsequent functions depending on that value undefined.

Even if you expect that the result of the command chain eventually shows up on screen in text form, just because your application happens to be that simple, and these functions are not used in any other scope, it’s still not safe to do it.

One more problem with your approach: It only “works” at all for string type functions. What with more complex datatypes, e.g. collections and alike?

For these, you can’t trivially encode the error case in the return value. Simply because there isn’t a solution with potentially different semantics, inevitably leading to undefined behavior later on.

How to capture exceptions correctly?

You only capture and swallow an exception ever, if you know that you can safely recover.

That means e.g. in the case of operations chained in LINQ style, you wrap no less than the whole chain into a try clause. The exception is supposed to interrupt the execution of the chain, to avoid undefined behavior.

And if the behavior of the surrounding scope still isn’t in and can’t be brought into a defined state in case of an exception, you let it bubble. As long until you can restore a sound state.

The completely broken error handling aside, decorating your functions to get a stream like semantic is perfectly legit.

Just be wary that this breaks with the coding style given by the .Net run time and most libraries. So the maintainability when enforcing such fundamentally different patterns is going to suffer. For one because that style is unexpected (-> principle of least surprise). For the other part because you can’t trivially interface with 3rd party code, but you would always need either a wrapper, or break your own patterns.

You should try to stick to the patterns and interfaces already present in your run time, if possible.

Leave a Reply Cancel reply