Use of optional parameter for execute in try block?

Question

I observed a dubious design pattern in some C# code today used in several methods:

public void DoSomething(bool safe=true) 
{
   if (!safe) DoSomethingDangerous();
   else 
   {
       try
       {
           DoSomethingDangerous();
       }
       catch
       {
           // Log error & carry on
       }
   }
}

I am wondering if this design pattern should be discouraged, and if so for what specific reasons. My own intuition tells me that these are possible concerns:

1. Masking errors without taking meaningful action
2. Encouraging the use of error handling for control flow (an antipattern)

However, I could potentially also see the utility in such a pattern. Perhaps it is useful to execute the logic in two different contexts one in which the error needs to be meaningfully dealt with, and another where it does not need to be worried about. This design pattern would help by creating a general method for both cases where you simply change the safe argument. Are there any legitimate reasons to use this design pattern?

Answer 1

The aspect of this that I'd say is the pattern to avoid, is the use of a flag argument.

The bool completely changes what the method does, so make it two methods:

public void DoSomethingSafely() 
{
    try
    {
        DoSomethingDangerous();
    }
    catch
    {
        // Log error & carry on
    }
}

public void DoSomethingDangerous()
{
    // whatever it does
}

Answer 2

First, is there a risk of

Masking errors without taking meaningful action ?

Probably not, since either the exception is passed to the caller (so the caller is responsible for a meaningful action), or the exception is logged, which is probably the intended action here.

Is there a sign of

use of error handling for control flow ?

The answer is IMHO also no, this would only be the case if DoSomethingDangerous throws an exception for a non-exceptional situation, but there is no indication for this in the code presented in the question.

So, if these two points are not the problem, is there another issue?

I guess the intention of the author was to make the code reusable in two different contexts:

• one where exceptions are allowed to pop-up to the caller, which for example could result in displaying a message box in the UI, which disrupts any outer processing

• one where exceptions shall not disrupt the outer control of flow (for example, in an asynchronous batch processing)

So far this could be a reasonable requirement. But is it possible to improve something? Of course, the obvious thing here is to avoid the code duplication (already suggested by @ErikLippert in a comment). If you need to keep the semantics, it should look like this:

public void DoSomething(bool safe=true) 
{
   try
   {
       DoSomethingDangerous();
   }
   catch(Exception ex)
   {
       if(!safe)
          throw ex;

       // Log error & carry on
   }
}

The other maintainability problem I see here is that the exception handling now happens at two different levels of abstraction, that is something I would avoid whenever possible, since it can become easily error prone. Solving this, however, requires to refactor the code first the way suggested by @DavidArno, and then refactor to outer code to call DoSomethingSafely only in the batch processing context and DoSomethingDangerous only in "UI mode".

This however, can result in a lot of refactoring. If there are several intermediate layers between the outer "batch controller" or "UI controller" and the shown method, it can become really hard to avoid additional code duplication inside the calling layer. The reason for this is, you will probably be forced to implement two different versions of the calling code, one for each context. One of these versions will use DoSomethingSafely in a specific way, and one using DoSomethingDangerous in a similar way, but with some differences in regards to error handling and maybe the other context. So to make a decision if that is the right approach, you need to look careful at the calling code.

Finally, here is another approach using Dependency Injection. The "Logger" itself could be an injected object (maybe it should not called "Logger" then, but something like "ExceptionProcessor"):

class MyClass
{
    IExceptionProcesser exProc;  // gets injected through the constructor

    public void DoSomething() 
    {
      try
      {
         DoSomethingDangerous();
      }
      catch(Exception ex)
      {
         // Log error, or rethrow
         exProc.Process(ex);   
      }
   }
}

That way, the parametrization of the exception behaviour is not done by a boolean parameter to the function any more, but by passing a different logger into the class. That fits typically better to the requirement of making the function behave differently in different contexts, and it avoids the usage of a flag argument as a function parameter as well.

Answer 3

I've used this sort of thing in a few places. Usually I frame it the opposite way (something is allowed to fail, the default is to throw); in particular it comes up doing some resolution operations when an app is starting up and it's unclear if the target is ready or not. Once everything is setup, it becomes a hard and fast error if the dependency is unavailable.

The other thing to consider with this pattern is refactoring. If I saw this code, I'd assume that it was originally

try { DoSomethingDangerous(); } catch { // log and continue }

and that was causing problems (or was otherwise undesirable). But changing that behavior would be a semantic change, and disruptive to consumers. By having the default safe flag it allows for testing the change with consumers to make sure it won't be very disruptive to change it fully. I've seen this sort of thing also done with feature flags or a similar configuration value rather than the function parameter.

Still, I'd consider this thing sort of smelly, and to be usually avoided.

Answer 4

My option is close to David Arno's, but I would consider the use of decorators. So it could go like

    interface A
    {
        void doSomething();
    }

    class UnsafeA : A
    {
        public void doSomething()
        {
            throw new Exception();
        }
    }

    class SafeA : A
    {
        private A origin;

        public SafeA(A a)
        {
            origin = a;
        }

        public void doSomething()
        {
            try {
                origin.doSomething();
            } catch (Exception e) {
                // handle
            }
        }
    }

So you could move the if logic closer to the program entry point (which is always a good thing, imho) and instantiate a class based on safe value. Besides that, I think this approach is more SRP-ish.

Answer 5

Improving OP's approach

My chief complaint is the use of the word "safe" (the call can still fail, you just won't know about it) and that it is too easy for the caller to make a mistake, because the default (safe=true) is to do something that is not really recommended.

The way I would have written it would have been:

public bool DoSomething(bool swallowExceptions = false) 
{
   try
   {
       DoSomethingDangerous();
       return true;
   }
   catch(System.Exception exception)
   {
       LogTheError(exception);
       if (!swallowExceptions) throw;
       return false;
   }
}

..thereby avoiding the duplicate code and ensuring that any exceptions that are swallowed are at least logged. Returning a boolean to indicate success is also helpful, and keeping with the pattern demonstrated by methods such as TryParse. The caller doesn't have to use it, of course.

Also, most importantly, the caller is forced to set swallowExceptions to true if they want it. That way we can be reasonably sure that it is on purpose.

Also, as I mentioned, the call isn't "safe" and that name doesn't tell the caller much, but calling it "swallowExceptions" is pretty clear, IMO.

Another way to do it is to split it into two methods, using the Try convention to make it clear to the caller what is going on:

public void DoSomething()
{
    //Do something
}

public bool TryDoSomething() 
{
   try
   {
       DoSomething();  //Calls the other function
       return true;
   }
   catch(System.Exception exception)
   {
       LogTheError(exception);
       return false;
   }
}

This way you don't need a separate, private DoSomethingDangerous method. You just have DoSomething and a call that wraps it.

A generalizable approach

On the other hand, it seems wasteful to write all this stuff for each and every method you want to make "safe." It could be done more generically like this:

class Try
{
    static public bool Run(Action action)
    {
        try
        {
            action();
            return true;
        }
        catch
        {
            return false;
        }
    }
}

Then you'd call DoSomethingDangerous either of these two ways:

Normal:

DoSomethingDangerous();

"Safe":

bool ok = Try.Run(DoSomethingDangerous);

or (if you need to pass arguments)

bool ok = Try.Run(() => DoSomethingDangerous("Hello"));

If you use this method, you won't have to monkey with the prototypes for any methods that you want to be "safe." You just wrap the call in Try.Run(). And it's pretty unlikely a programmer would do it by accident.

Answer 6

One reason why I'd avoid this is because it exposes internal method details through the interface. The caller needs to know what the method does, not whether or not it's going to eat exceptions. If the caller wants to handle exceptions or ignore them, it can determine that or let the exception bubble up to some point where that determination is made.

Using such a flag violates SRP in a sense. The class should only responsible for whatever it does, not for knowing what the caller wants to do with its exceptions. This is especially true since it's trivially easy for the caller to manage what - if anything - it does with an exception raised from the method.

And it's only a matter of time before someone realizes that the method contains multiple operations that could throw exceptions and takes it to the next level:

public void DoSomethingDangerous(
    bool safe = true, 
    bool throwNullReference = false, 
    bool handeIoException = true)