3

I've been tasked with refactoring a console application, which is constantly running on a server and receiving messages from a service bus.

Right now, it just parses the incoming message, and based on a property, will use a switch statement to call one of many different functions (about 70 at the moment, always growing). One problem is that if a function fails, it's not retried. Not to mention just the ugliness of one giant switch statement.

I'm leaning towards using the Command Pattern to rectify this (https://scottlilly.com/c-design-patterns-the-command-pattern/), but have also considered a pub/sub pattern to handle these functions.

Anybody know what the best architecture pattern might be for this situation?

Steven
  • 139
  • 5
    What you describe sounds like something that can be handled with plain dictionary with key being string and value being a delegate. But you don't provide any details to decide if this solution is truly viable. – Euphoric Jan 31 '18 at 21:20
  • Or perhaps a combo of what Euphoric described and the command pattern. The command pattern will unify your commands under a single abstract interface for execution, at which point you can then map them to a key of some sort (whatever you are using to switch). –  Jan 31 '18 at 22:04
  • 2
    One thing you might want is that the components register themselves, so there is no universal switch statement or builder of the dictionary to maintain separately from the components that have the handlers. I suppose that goes to the pub part of pub/sub... – Erik Eidt Feb 01 '18 at 01:56
  • Do the messages fall into broader categories, like "things done to user accounts," "things done to inventory," "things done to widgets"? Or is it just currently 70 unrelated commands? Even for a 70-item switch statement, breaking it into 10 groups of 7 commands (or whatever) might make it more maintainable. But I think it would also open up other possibilities. However we'd need more info to say anything truly useful about it. – user1118321 Feb 01 '18 at 03:48
  • 1
    Possible duplicate of Refactoring Switch Statements and is there any real use for Switch Statements at all? – gnat Feb 01 '18 at 05:38
  • 1
    @TeamUpvote, C# already has that command pattern built in: they are called delegates. Why re-invent the wheel and implement something different? – David Arno Feb 01 '18 at 10:02
  • @DavidArno Similar thing with function objects and so forth in C++ and agreed that if all you need is like execute and nothing else, it's better than introducing a whole new interface just for one function. I tend to see that as an application of the command pattern so I think delegates are just fine as a way to implement that -- sorry if I didn't make that clear. Where I tend to find making an interface useful is that they might have a few more functions, like undo or redo. Otherwise I also prefer function objects/delegates/function pointers over functionids... don't like seeing [...] –  Feb 01 '18 at 18:26
  • @DavidArno ... a bunch of interfaces with just one function when these alternatives could have been used instead... like ITreeVisitor -- could just be a function object which could be more easily constructed using lambdas! So I often don't like how design patterns can sometimes tempt people to implement them "by the book" this way while ignoring the language features. I just tend to take it as a given that many of these can and should be implemented with the most appropriate language features. –  Feb 01 '18 at 18:28

3 Answers3

4

Since you have tagged this question with C#, I'll offer the language-specific answer to your question. C# and the BCL offer a built-in version of the command pattern: a dictionary of delegates. Rather than have, for example:

SomeType Foo(MessageType message)
{
    switch (message.Id)
    {
        case "Id1" : return HandleId1();
        case "Id2" : return HandleId2();
        ...
    }
}

You replace it with

private readonly Dictionary<string, Func<SomeType>> _handlers =
    new Dictionary<string, Func<SomeType>>
    {
        ["Id1"] = HandleId1,
        ["Id2"] = HandleId2,
        ...
    };

...

SomeType Foo(MessageType message) => _handlers[message.Id]();
David Arno
  • 39,270
  • The big difference between you two examples is what happens when someone sends you a new message.Id without modifying the supporting code. The first example ignores it while the second one crashes because of it. Either behavior could be preferable. – Peter M Feb 01 '18 at 12:39
  • That's what "default" in a switch statement is there for. Even beginners would know that. – gnasher729 Feb 04 '18 at 14:45
1

Given that you are using a service bus already I would split the app into many apps and the message queue into a queue per type.

That way you your app handles only one type of message, enabling you to scale them separately and add new types without redeploying.

You can use the service bus error queues for retrying failed messages

Ewan
  • 75,506
1

The answer given by @David Arno is what I would do, with one difference. I would not use delegates, but would rather go with Command objects. So, you would have something like this:

Dictionary<string, ICommand> _handlers = new Dictionary<string, ICommand>();

where

public interface ICommand
{
    void Execute();
}

Then, you could implement a null object like this:

public NullCommand : ICommand
{
    public void Execute()
    {
    }
}

and your code would look like this:

ICommand GetCommand(string id)
{
    ICommand retVal = null;
    if (!_handlers.TryGet(id, out retVal))
    {
        retVal = new NullCommand();
    }
    return retVal;
}

This way, the ownership is clear and testability is greater.

Vladimir Stokic
  • 2,973