1
\$\begingroup\$

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:

  1. 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();
    
  2. 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.

\$\endgroup\$
  • \$\begingroup\$ Welcome to Code Review! I'm afraid this question does not match what this site is about. Code Review is about improving existing, working code. The example code that you have posted is not reviewable in this form because it leaves us guessing at your intentions. Unlike Stack Overflow, Code Review needs to look at concrete code in a real context. Please see Why is hypothetical example code off-topic for CR? \$\endgroup\$ – Mathieu Guindon Aug 30 '16 at 1:42
  • \$\begingroup\$ Hi @Mat'sMug! I've done a couple edits to show a snippet of the actual code in use. The class i'm trying to improve has +15 methods like that and everyone of them uses the same pattern. I hope i've improved the question enough but if further clarification is needed let me know. \$\endgroup\$ – Kemuel Sanchez Aug 30 '16 at 2:41
  • \$\begingroup\$ A topic starting with How to do something usually suggests there is something wrong with the code or it isn't working yet. We require woking code here. Please adjust it so the purpose of the code is clear. \$\endgroup\$ – t3chb0t Aug 30 '16 at 4:01
  • \$\begingroup\$ As to the try/catch. If you have a lot of methods like this one then the performance might suffer. Why don't you try to open the registry key and see if it's null and then read the value etc. without any exeption catching. \$\endgroup\$ – t3chb0t Aug 30 '16 at 4:03
  • \$\begingroup\$ Hi @t3chb0t, As for the performance on the try/catch it isn't that critical: the whole thing runs as a background task and sometimes involve operations that have a lot of latency themselves (wmi queries and such). So, I'm willing to give up a little performance for conciseness :) \$\endgroup\$ – Kemuel Sanchez Aug 30 '16 at 10:34
3
\$\begingroup\$
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.

| improve this answer | |
\$\endgroup\$
  • \$\begingroup\$ Hello @Ext3g. You did a great job pointing out why exceptions exist and what is the correct way to use them is the best practice; however I'd like to point out that the code does take the first approach you mention: Swallowing the exception and continuing with a safe default. In my case, it is useful to get the exception as a string. Why? Well, it documents why I couldn't fetch the data. So, in the database, i can easily know what properties were successfully fetched and which one were not. I really prefer that to having "blank" or "default" on the corresponding column. \$\endgroup\$ – Kemuel Sanchez Aug 30 '16 at 12:54
  • \$\begingroup\$ Also, it is only ever supposed to handle string types. I'm only gathering getting textual information and sending that to a database. \$\endgroup\$ – Kemuel Sanchez Aug 30 '16 at 12:56

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service, privacy policy and cookie policy

Not the answer you're looking for? Browse other questions tagged or ask your own question.