Using Action dictionaries instead of switch statements

Question

I'm just reviewing some of my old code (have some spare time), and I noticed a rather lengthy switch statement. Due to gaining new knowledge, I have since refactored it in the following form:

private Dictionary<string, Action> createView
    {
        get
        {
            return new Dictionary<string, Action>()
            {
                {"Standard", CreateStudySummaryView},
                {"By Group", CreateStudySummaryByGroupView},
                {"By Group/Time", CreateViewGroupByHour}
            };
        }
    }

Would you consider this good practise, or is this simply a case of being superflous and unneccessary? I'm keen to ensure new techniques that I learn, are not being clever just for the sake of it, and that they actually add benefit to the code.

Thanks.

Andrew Shepherd · Accepted Answer · 2011-08-24T10:24:08.560

21

Long switch statements are a classic bad smell, and are always a target for refactoring.

The "standard" step to perform here is Replace Conditional with Polymorphism. This was one of the steps listed in Martin Fowler's book Refactoring (published 11 years ago in 1999).

Now that it's so easy to treat functions like objects (eg with Action) this might be just as good a solution.

And no, I don't think you're being clever for the sake of it. If I wanted to add another option in the future, I can easily see what needs to be done.

edited Aug 24 '11 at 10:24

answered Aug 24 '11 at 10:16

Andrew Shepherd

44,254
30
139
205

+1 Thanks for the link, I've never come across that refactoring catalogue before - I'll give it a read. – Adam Houldsworth Aug 24 '11 at 10:23
I thought this wasn't the answer I was looking for, but after thinking about it, it was. I have an automatically generated file with thousands of switch cases, each one executing a method. It is for a serializable event system for the Unity engine. Now I have thousands of classes that inherit an abstract class with an Invoke virtual function and a dictionary that contains them replacing the switch. Now a caller can cache the inherited class that gets from the dictionary and call its virtual function instead of going though a switch for every call. – Forestrf Jan 11 '20 at 04:01

score 11 · Answer 2 · edited Apr 27 '23 at 04:26

Depending on your app you can avoid to always construct a new dictionary object, but declare it like a class member, initialize on first access and always return the same instance. But it's hard to say, if it will actually fit your needs. Like this I mean

public class MyClass
{
    Dictionary<string, Action> dict = null;

    private Dictionary<string, Action> createView
    {
        get
        {
            if (dict == null)
            {
                dict = new Dictionary<string, Action>()
                {
                    {"Standard", CreateStudySummaryView},
                    {"By Group", CreateStudySummaryByGroupView},
                    {"By Group/Time", CreateViewGroupByHour}
                };
            }

            return dict;
        }
    }
}

or use the auto property initial value since C# 6:

public class MyClass
{
    private Dictionary<string, Action> createView { get; } = new()
    {
        {"Standard", CreateStudySummaryView},
        {"By Group", CreateStudySummaryByGroupView},
        {"By Group/Time", CreateViewGroupByHour}
    };
}

EDIT

From conceptual point of view, me replacing long swicth/case with dictionary TryGetValue is a pretty good solution.

Hope this helps...

score 5 · Answer 3 · answered Aug 24 '11 at 10:29

5

This approach is excellent.

I use it with more than just Action. It's also quite effective for filters and selectors. Something like:

var filters = new Dictionary<string, Func<MyEntity, bool>>()
{
    // ...
};

var query = entities.Where(filters["X"]);

answered Aug 24 '11 at 10:29

Enigmativity

113,464
11
89
172

That's a good approach also. Having learnt the basics of the language, i'm a lot more comfortable now with learning more of the complex parts, and it's fun discovering these little tricks :) – Darren Young Aug 24 '11 at 10:30

Adam Houldsworth · Answer 4 · 2011-08-24T10:22:47.067

If the code, once written, is largely static and not subject to too much change then I'd have stuck with a switch. Your dictionary approach here, on the surface at least, lends itself nicely to being more dynamic - that is more requirements based though.

As for replacing switches everywhere with code using this approach, I'd personally not do that in most cases. My honest opinion is that would just be being clever for the sake of it, but it is still easily maintainable. Personal taste over best practise is the largest factor here, as I see it.

On the other hand, as others have said, this could be a workable solution to long switch statements. Then again something like the Strategy Pattern would also be a good way to support changes in behaviour.

Using Action dictionaries instead of switch statements

4 Answers4

Linked

Related