Looking for an effective pattern to cope with switch statements in C#

Question

I sometimes end up with services encapsulating the responsibility of doing some sort of business process for which there are several possible outputs. Typically one of those output is success and the others represent the possible failures of the process itself.

To fix the idea consider the following interfaces and classes:

  interface IOperationResult 
  {
  }

  class Success : IOperationResult 
  {
    public int Result { get; }
    public Success(int result) => Result = result;
  }

  class ApiFailure : IOperationResult 
  {
    public HttpStatusCode StatusCode { get; }
    public ApiFailure(HttpStatusCode statusCode) => StatusCode = statusCode;
  }

  class ValidationFailure : IOperationResult 
  {
    public ReadOnlyCollection<string> Errors { get; }

    public ValidationFailure(IEnumerable<string> errors)
    {
      if (errors == null)
        throw new ArgumentNullException(nameof(errors));

      this.Errors = new List<string>(errors).AsReadOnly();
    }
  }

  interface IService 
  {
    IOperationResult DoWork(string someFancyParam);
  }

The classes consuming the IService abstraction are required to process the returned IOperationResult instance. The straightforward way to do so is writing a plain old switch statement and decide what to do in each case:

      switch (result) 
      {
        case Success success:
          Console.WriteLine($"Success with result {success.Result}");
          break;

        case ApiFailure apiFailure:
          Console.WriteLine($"Api failure with status code {apiFailure.StatusCode}");
          break;

        case ValidationFailure validationFailure:
          Console.WriteLine(
            $"Validation failure with the following errors: {string.Join(", ", validationFailure.Errors)}"
          );
          break;

        default:
          throw new NotSupportedException($"Unknown type of operation result {result.GetType().Name}");
      }

Writing this type of code in different points of the codebase quickly generates a mess, because this basically violates the open closed principle.

Each time the implementation of IService gets modified by introducing a new implementation of IOperationResult there are several switch statements that must be modified too. The developer implementing the new feature must be aware of their existence, unless there are well written tests which can automatically detect the missing modifications in the points where the code switches over IOperationResult instances.

Maybe the switch statement can be avoided at all.

This is easy to do when IService is used for one specific purpose. As an example, when I write ASP.NET core MVC controllers in order to keep action methods simple and lean I inject a service in the controller and delegate to it all the processing logic. This way the action method only cares about handling the HTTP request, validating the parameters and returning an HTTP response to the caller. In this scenario the switch statement can be avoided from the beginning by using polymorphism. The trick is modifying IOperationResult this way:

  interface IOperationResult 
  {
    IActionResult ToActionResult();
  }

The action method simply calls ToActionResult on the IOperationResult instance and returns the result.

In some cases the IService abstraction must be used by different callers and we need to let them the freedom to decide what to do with the operation result.

One possible solution is defining one higher order function, lets call it processor for simplicity, having the responsibility of processing a given instance of IOperationResult. It's something like this:

  static class Processors 
  {
    static T Process<T>(
      IOperationResult operationResult,
      Func<Success, T> successProcessor,
      Func<ApiFailure, T> apiFailureProcessor,
      Func<ValidationFailure, T> validationFailureProcessor) => 
        operationResult switch
        {
          Success success => successProcessor(success),
          ApiFailure apiFailure => apiFailureProcessor(apiFailure),
          ValidationFailure validationFailure => validationFailureProcessor(validationFailure),
          _ => throw new ArgumentException($"Unknown type of operation result: {operationResult.GetType().Name}")
        };
  }

The advantages here are the following:

• there is only one point where the switch statement is done
• each time a new implementation of IOperationResult is defined there is only one point that needs to be modified. Doing so the signature of the Process function gets modified too.
• the modification done at the previous point produces several compile time errors where the Process function gets called. This errors must be fixed, but we can trust the compiler being able to find all the points to be modified

A more object oriented alternative is modifying the definition of IOperationResult by adding one method per each intended usage of the operation result, so that the switch statement can be avoided once more and the only thing to do is actually writing a new implementation of the interface.

This is an example in the hypothesis that there are two different consumers of IService:

  interface IOperationResult 
  {
    string ToEmailMessage(); // used by the email sender service
    ICommand ToCommand(); // used by the command sender service
  }

Any thoughts ? Are there other or better alternatives ?

Answer 1

The problem

The purpose of having these result classes derive from the same interface is so that the interface becomes what the consumer knows and works with. The consumer doesn't care about the specific implementing classes.

However, your interface doesn't contain anything. You're using it as a marker interface. If I, as a consumer, receive an IOperationResult object, what can I do with it? Nothing. Because the IOperationResult interface defines no contract whatsoever.

That defeats the purpose of having your result classes share the same interface. You've essentially enforced them all to comply to the same contract, but put an empty contract in place so it's literally impossible to not comply with it.

---

Your first solution

Your Processors class is effectively trying to reinvent the event delegation wheel. You're defining specific event handlers (the Func objects) that handle every outcome.

But this still violates OCP for the exact same reason. You still have the switch which needs to be expanded whenever a new result type is developed. All consumers will still need to add a new handler for a new result type.

It's still the same problem. You've just obfuscated it with some additional complexity.

---

Your second solution

You've found a new way to violate OCP. Now, instead of having to expand the code every time a result type is developed, you're having to expand the interface whenever a new consumer is developed.

It's the same problem all over again.

On top of that, now your core logic needs to somehow know how each consumer wants its own result to be handled, and your core logic is going to have to pre-chew the result exactly how every particular customer wants it.
This will lead to sheer insanity in your core logic that now has to account for the handling of its own outcome (based on every individual consumer's needs); which means you're violating SRP on top of OCP.

---

My proposed solution

Overall, it seems like you've quite understood the core issue of OCP, as you've not avoided it in any of the solutions you've claimed were solutions.

How you develop your interface correctly depends on what you want to do with it. Based on your usage example in the switch case, it seems you're primarily interested in two things: whether it was a success, and a possible message to inform the consumer further. Taking that, your interface becomes straightforward:

public interface IOperationResult
{
    bool IsSuccess { get; }
    string Message { get; }
}

And your implementations become straightforward:

public class Success : IOperationResult
{
    public bool IsSuccess => true;
    public string Message { get; set; }
}

public class ApiFailure : IOperationResult
{
    public bool IsSuccess => false;
    public string Message { get; set; }
}

However, in the interest of not overengineering, there is a simpler approach here. Based on your current needs, you don't really need separate classes. The only thing that's different is going to be the values contained in the result object, not the structure of the result object itself.

It's a lot cleaner here to do away with the interface and simply use a straightforward DTO class. What you now call derivations (success, API failure, ...) can be expressed as static methods, which you essentially use as "constructors with a name", like so:

public class OperationResult
{
    public bool IsSuccess { get; private set; }
    public string Message { get; private set; }

    //This ensures you can only instantiate an object via the static methods
    private OperationResult() {}

    public static OperationResult Success()
    {
        return new OperationResult()
        {
           IsSuccess = true,
           Message = String.Empty
        };
    }

    public static OperationResult ValidationFailure(string message)
    {
        return new OperationResult()
        {
           IsSuccess = false,
           Message = message
        };
    }
}

Which you can then use wherever you need it:

if( a == b )
    return OperationResult.Success();
else
    return OperationResult.ValidationFailure("a does not equal b");

In this example, I made it so that a message would only be given for a failure. That's just an example of how you can force certain outcomes to have certain values, which enables you to set your resulting values exactly how you want them in every case.

For the situation you described in the question, the above class suffices. As a general rule, don't overengineer simple things. It's just going to cost time and effort now, and in the future when you have to maintain it.

Answer 2

It seems to me that you are massively over complicating your life in this example by not using Exceptions instead of your IOperationResult interface.

Simply throw a ValidationException or APIErrorException in your Service and handle with try catch as usual.

Furthermore, say you did that, I would then say you should not be exposing the internals of your Service by handling the specific type of error outside of it.

ie your Service throws an APIException or returns an APIFailure and your process as a whole needs that to be handled in a specific way. But the calling code shouldn't care whether the Service uses an API or not to achieve its result.

The Service should simple throw an Exception and any special logic that needs to be implemented on Exceptions due to API failures should be dealt with in that Service class, perhaps by injecting a Handler.

if you rearrangew your service like this, you will no longer require select case or try catch switches in the calling code

public class MyService : IService<Result>
{
    private IApiErrorHandler apiErrorHandler;
    public Result Process()
    {
        try
        {
            /call api
        }
        catch(Exception ex)
        {
            apiErrorHandler(ex)
            throw;
        }
        return result;
    }
}

public class MyController
    public IActionResult  MyAction
    {
         return myService.Process(); //rely on error handler to convert errors to http/json
    }

If its purely the formatting of the Exception returned to the client which changes, perhaps a different HTTP code for validation errors? then you can handle that all in the Error Handler, or simply check the validation before calling the Service

Answer 3

This is a long but very interesting question, where you have exposed your step by step research.

To summarize the problem:

• IService does some processing and returns an IOperationResult
• Different post-result actions need to be performed depending on IOperationResult
• Actions cannot be implemented polymorphically in IOperationResult because concrete actions depend on the calling context.
• As a work around, a lot of switch are used, but this is a problem in view of OCP, as well as perhaps the LoD

Unfortunately, there is no silver bullet for solving this problem in a general way: the fact that actions depend on context AND result, means that you must know about both to decide the action. Combinatorial explosion.

You can perhaps find a better place or a better way to make the decision, but you always will have to add new actions if there are new IOperationResult or if there is a new context invoking the service.

If you have families of predefined actions that need to be performed in several places, you could replace the switch with some kind of predefined chain of responsibility to handle the IOperationResult. But if the actions are different in every switch, this would just be an overhead and instead of modifying switches you'd be adding handlers. The switch would seem a very decent approach then.

For this kind of problems, it may be worth considering an event driven design. The event would provide information on a flow (i.e. sequence of related service invocations, with some status information), a processor would dequeue events and pass them to an event handlers (again, chain of responsibility, or, a table logic). THe event would either indicate services to be invoked or results to be digested. It doesn't make the whole thing simpler. But when you add a new kind of IOperationResult, you'd just add a couple of new handlers (one for each different kind of actions to be performed), and the event based architecture will do the rest.

Answer 4

Flaters answer, whilst correct for the question, does not really solve the general issue.

What if you want to implement the same pattern but with a strongly typed 'message' instead of a string? E.g. IOperationResult becomes IOperationResult<T>.

What if you want to project this 'message' to a different type once it's wrapped in the generic parameter?

One solution (pointed out by Ewan) is exception programming flow, which is generally considered bad practice and not favoured by the ASP.NET team. It also only get's you one "layer" up the callstack. Depending on the complexity of the application this may have to be caught and thrown several times creating more surface area for missed catches. Bleuh.

The Visitor (anti-)Pattern could be used but relies on various assumptions about the architecture of the solution. For example both the implementations of the IOperationResult<T> and the visiting class would have to live in the same assembly in order to have awareness of each other.

The solution you propose, whilst ugly, works and may be a necessary trade off in C# in it's current form (8.0). There is no nice solution in C# whilst it does not support discriminated unions or closed types. There is a proposal.

The best you can do to mitigate risks of new implementations is to limit where the switch occurs, preferably at the very boundaries of the application, define common extensions for projecting the type and carefully consider where you're using the pattern.

Answer 5

Worth noting the Open / Close principal is only a benefit in larger code bases eg for micro services its likely to add unneeded complexity and violate KISS.

2 decades ago i dropped switch statements, lately i'm using them in smaller code bases ( more c than C++ like) along with public pure static methods and i'm happier with the result.