Simplyfing code with a lot of ifs

Question

I have a method which based on the enum, and to be clear at start we have this situation:

public void MyMetohd(Somestatus status)
{
if(status == Somestatus.Enum1)
{
DoA();
DoB();
DoC();
DoD();
DoE();
}
if(status == Somestatus.Enum2)
{
DoA();
DoB();
DoC();
DoD();
}

if(status == Somestatus.Enum3)
{
DoA();
DoB();
DoC();
}

if(status == Somestatus.Enum4)
{
DoA();
DoB();
}

if(status == Somestatus.Enum5)
{
DoA();
}
}

How would you optimize this kind of code ( it isn't mine)?

Are you talking about a plain vanilla enum, or is it a flag? — Ragesh, Jan 02 '11 at 17:24
What do you mean by optimize? What exactly are your performance bounds and what is limiting it at the moment? — David Heffernan, Jan 02 '11 at 17:27
I wonder why no one mentioned Duff's Device: http://en.wikipedia.org/wiki/Duff%27s_device — ruslik, Jan 03 '11 at 09:18
Check if you can't refactor your code to use a [Strategy Pattern](http://en.wikipedia.org/wiki/Strategy_pattern). (Check [this](http://ootips.org/strategy-vs-case.html) as well). You can combine it with a factory method or factory class. — Frederik Gheysels, Jan 02 '11 at 17:21

zsalzbank · Answer 1 · 2011-01-03T01:40:24.713

5

You can use a comparison if you set the values of each member of the enum.

enum Somestatus : int
{
    Enum1 = 1,
    Enum2 = 2,
    ...
}

Then just use comparison to do your code. Because you always do DoA(), start with that.

if(status <= Somestatus.Enum5)
    DoA();

if(status <= Somestatus.Enum4)
    DoB();

if(status <= Somestatus.Enum4)
    DoC();
...

Keep on going like that. This way all your functions will be called when the value is Enum1.

edited Jan 03 '11 at 01:40

answered Jan 02 '11 at 17:28

zsalzbank

9,140
1
23
38

Good example, I think you meant This way all your functions will be called when the value is Enum1 instead, as Enum1 is the only scenario when all 5 methods should be executed. – Andrew T Finnell Jan 02 '11 at 18:49
I think that such 'solution' is hard to read, and will be very hard to maintain. – Frederik Gheysels Jan 02 '11 at 19:13
++ That's the way I would do it. – Mike Dunlavey Jan 03 '11 at 01:17

Brook · Answer 2 · 2011-01-02T18:20:29.243

By optimize I'll assume you mean "make DRYer".

You're going to have to strike a balance between code which is easy to read (which, what you have is, although slightly repetitive) and code which repeats as little as possible

Just typing this makes me feel dirty, but if what you want is DRY and fewer LOC, I think it would do what you want.

switch (status)
            {
                case Somestatus.Enum1:
                    DoE();
                    goto SomeStatus.Enum2;
                case Somestatus.Enum2:
                    DoD();
                    goto SomeStatus.Enum3;
                case Somestatus.Enum3:
                    DoC();
                    goto SomeStatus.Enum4;
                case Somestatus.Enum4:
                    DoB();
                    goto SomeStatus.Enum5;
                case Somestatus.Enum5:
                    DoA();
                    break;
                default:
                    throw new InvalidArgumentException("Unknown Status");
            }

score 3 · Answer 3 · edited May 23 '17 at 12:18

You can use a dictionary keys on the enum value and with a list of Action or Action<T> to execute.

Dictionary<int,IList<Action>> actionsPerEnumValue;

Populate this dictionary with the enum values and the actions for each.

In your function get the list of functions per value and invoke each action.

foreach(var act in actionsPerEnumValue[status])
{
    act();
}

See this SO answer for an example.

score 0 · Answer 4 · answered Jan 02 '11 at 17:20

0

At first glance a switch statement would seem to be the best approach, but you've got a lot of repeated code even in that:

switch (status)
{
    case Somestatus.Enum1:
        DoA();
        DoB();
        DoC();
        DoD();
        DoE();
        break;
    case Somestatus.Enum2:
        DoA();
        DoB();
        DoC();
        DoD();
        break;
    ...
}

While this is better it's still not ideal as you have the calls to DoA etc repeated.

answered Jan 02 '11 at 17:20

ChrisF

127,439
29
243
315

1

Except without breaking. (Wait, does C# allow case statements to fall through without break?) – BoltClock Jan 02 '11 at 17:20
If you omit the break; will it carry on to the other values of status? – m.edmondson Jan 02 '11 at 17:22
@BoltClock - no you can't fall through in C#, you have to use `goto`. – ChrisF Jan 02 '11 at 17:23
@BoltClock - No, it doesn't :( – Oded Jan 02 '11 at 17:23
@ChrisF, @Oded: Oh, that blows :( – BoltClock Jan 02 '11 at 17:24
C# does allow you to fallthrough in switch statements. Just verified :) – Frederik Gheysels Jan 02 '11 at 17:27
2

@Frederik - not if you have code inbetween the cases though - "Unlike C++, C# does not allow execution to continue from one switch section to the next." http://msdn.microsoft.com/en-us/library/06tc147t(v=VS.100).aspx – ChrisF Jan 02 '11 at 17:29
@BoltClock: Not it does not, you must specify a jump statement. The best you could do would be use a goto Somestatus.Enum2 (Where Enum2 is the next in line) – Brook Jan 02 '11 at 17:40

score 0 · Answer 5 · answered Jan 02 '11 at 17:23

0

when you are working on same condition and different value for that ,at that time better u use switch..case... Use If..else when you want to test multiple conditions.

answered Jan 02 '11 at 17:23

Kishan Gajjar

1,110
3
20
41

score 0 · Answer 6 · answered Jan 02 '11 at 17:24

you could also "invert" the logic if that makes it simpler (depends on the number of enums to cover vs. the number of different actions):

if(status == Somestatus.Enum1 || status == Somestatus.Enum2)
 DoA();

if(status == Somestatus.Enum1 || status == Somestatus.Enum4)
 DoB();

...

score 0 · Answer 7 · answered Jan 02 '11 at 17:26

0

sounds like you should use State Pattern

answered Jan 02 '11 at 17:26

Sebastian Piu

7,490
1
30
49

Pratik Deoghare · Answer 8 · 2011-01-02T18:02:38.070

0

EDITED:

// ANOTHER WAY
public void MyMetohd(Somestatus status)
{
    switch(status)
    {
        case Somestatus.Enum1:
             do_("ABCDE");
             break;
        case Somestatus.Enum2:
             do_("ABCD");
             // and so on...
        }
}

public static void do_(string s)
{
    foreach(char ch in s)
    {
        switch(ch)
        {
            case 'A':
                 doA();
                 break;
            case 'B':
                 doB();
                 break;
            case 'C':
                 doC();
                 break;
            case 'D':
                 doD();
                 break;
            case 'E':
                 doE();
                 break               
        }
    }
}

edited Jan 02 '11 at 18:02

answered Jan 02 '11 at 17:41

Pratik Deoghare

30,271
27
94
143

2

Your first way doesn't compile, as mentioned previously, c# requires an explicit jump statement, and does not fall through. – Brook Jan 02 '11 at 17:54
@Brook: Thanks man! I will never forget this now :) +1 – Pratik Deoghare Jan 02 '11 at 18:03
No problem, I thought the same thing at first too, along with a bunch of other people. The other thing is that you're limited in what jump statements you can use (break or goto). It would be nice if they'd let you use "continue" to explicitly fall through. – Brook Jan 02 '11 at 18:25
Hummm... anyways I think the best thing would be to use some design pattern as has suggested by @Frederik Gheysels. – Pratik Deoghare Jan 02 '11 at 19:38

Andrew T Finnell · Answer 9 · 2011-01-02T18:59:38.643

Using a pattern to solve this will be the best solution like the others have suggested. I wanted to offer yet another solution.

I highly recommend NOT doing this though.

public void MyMetohd(Somestatus status)
    DoA();
    if (status != SomeStatus.Enum5) {
        DoB();
        if (status != SomeStatus.Enum4) {
            DoC();
            if (status != SomeStatus.Enum3) {
                DoD();
                if (status != SomeStatus.Enum2) {
                    DoE();
                }
            }
        }
    }
}

score 0 · Answer 10 · answered Jan 02 '11 at 19:30

Although I'd go for the solution of Brook myself, I would like to point out another elegant and short solution.

public void MyMethod(Somestatus status)
{
    foreach (Action toDo in new Action[] { DoA, DoB, DoC, DoD, DoE }.Take(5 - (int)status))
        toDo();
}

However, this assumes Somestatus is defined like this:

enum Somestatus
{
    Enum1,
    Enum2,
    Enum3,
    Enum4,
    Enum5
}

I like this solution as an academic and because is very short, but it certainly does not have great readability or maintainability.

Simplyfing code with a lot of ifs

10 Answers10