If Condition inside switch case

Question

I am trying to convert an if statement to switch cases (for readability)

1) I've read switch statements are aweful in general - Is that true? https://stackoverflow.com/questions/6097513/switch-statement-inside-a-switch-statement-c

2) The statement goes like this:

switch (Show)
                {
                    case Display.Expense:
                        if (expected.EXPENSE != true)
                            break;
                    case Display.NonExpense:
                        if (expected.EXPENSE == true)
                            break;
                    case Display.All:
                        //Code
                        break;
                }

Error is:

Control cannot fall through from one case label ('case 1:') to another

This is the original if statement:

if ((Show == Display.All) || (expected.EXPENSE == true && Show == Display.Expense) || (expected.EXPENSE == false && Show == Display.NonExpense))
{
    //Code
}

Consider this question about the error. http://stackoverflow.com/questions/6696692/control-cannot-fall-through-from-one-case-label — Dmitrii Dovgopolyi, Apr 01 '13 at 11:35
`Switch` is for **different** behavior on different cases. You have **only one** behavior on diffrenet cases, so as @Dennis wrote you don't need the `switch`. — Dmitrii Dovgopolyi, Apr 01 '13 at 11:39
Its not clear what you are trying to acomplish by using an if statement that does nothing except `break` the switch case. If you want to use an if statement within a case condition you need an else condition which also does a `break` — Security Hound, Apr 01 '13 at 13:08
was going for fallthrough, ie if condition = true then fall-through else break for the switch case — AngelicCore, Apr 01 '13 at 13:48

Eric Lippert · Answer 1 · 2013-04-02T13:47:53.140

First off, I notice that you forgot to ask a question in your second point. So I'm going to ask some questions for you addressing your second point:

What is the meaning of the "can't fall through" error?

Unlike C and C++, C# does not allow accidental fall-through from one switch section to another. Every switch section must have an "unreachable end point"; it should end with a break, goto, return, throw or (rarely) infinite loop.

This prevents the common bug of forgetting to put in the break and "falling through" accidentally.

You've written your code as though fall-through is legal; my guess is that you're a C programmer.

How can I force fall-through in C#?

Like this:

switch (Show)
{
case Display.Expense:
    if (expected.EXPENSE != true)
        break;
    else
        goto case Display.All;
case Display.NonExpense:
    if (expected.EXPENSE == true)
        break;
    else  
        goto case Display.All;
case Display.All:
    //Code
    break;
}

Now the reachability analyzer can determine that no matter which branch of the "if" is taken, the switch section endpoint is unreachable.

Is this good style?

No. Your original code was a lot more readable.

I've read switch statements are aweful in general - Is that true?

Opinions vary. Switch statements are very useful when there is a small number of very "crisp" alternatives whose behaviours do not interact in complex ways. Some people will tell you that switched logic should instead be handled by virtual methods or visitor patterns, but that can be abused as well.

Should I use a switch in this particular case?

I wouldn't.

How would you improve my code?

if ((Show == Display.All) || 
    (expected.EXPENSE == true && Show == Display.Expense) || 
    (expected.EXPENSE == false && Show == Display.NonExpense))
{
    //Code
}

First off, don't name things IN ALL CAPS in C#.

Second, don't compare Booleans to true and false. They're already Booleans! If you want to know the truth of statement X you would not say in English "is it true that X is true?" You would say "Is X true?"

I would likely write:

if (Show == Display.All || 
    Show == Display.Expense && expected.Expense || 
    Show == Display.NonExpense && !expected.Expense)
{
    //Code
}

Or, even better, I would abstract the test away into a method of its own:

if (canDisplayExpenses())
{ 
    //Code
}

Or abstract the whole thing away:

DisplayExpenses();

score 9 · Accepted Answer · answered Apr 01 '13 at 11:40

The compiler will not understand what you mean here.

switch (Show)
{
    case Display.Expense:
        if (expected.EXPENSE != true)
            break;
        // missing break here
    case Display.NonExpense:

The compiler will not connect the dots and understand that the break; statement inside your if statement is linked to the switch statement. Instead it will try to link it to a loop, since break; statements on their own can only be used with loops, to break out of it.

That means that your case block is missing its break statement to complete it, and thus the compiler complains.

Instead of trying to wring the necessary code out of a switch statement, I would instead break up your original if statement.

This is yours:

if ((Show == Display.All) || (expected.EXPENSE == true && Show == Display.Expense) || (expected.EXPENSE == false && Show == Display.NonExpense))
{
    //Code
}

This is how I would write it:

bool doDisplayExpected =
       (Show == Display.All)
    || (Show == Display.Expense    && expected.EXPENSE)
    || (Show == Display.NonExpense && !expected.EXPENSE);
if (doDisplayExpected)
{
    // code
}

You don't have to pack everything on one line.

Also, I would try to name properties so that they're easier to read, I would rename the EXPENSE property to IsExpense so that the above code would read like this:

bool doDisplayExpected =
       (Show == Display.All)
    || (Show == Display.Expense    && expected.IsExpense)
    || (Show == Display.NonExpense && !expected.IsExpense);
if (doDisplayExpected)
{
    // code
}

Then, ideally, I would refactor out the sub-expressions to methods:

bool doDisplayExpected =
       ShowAll()
    || ShowExpense(expected)
    || ShowNonExpense(expected);
if (doDisplayExpected)
{
    // code
}

public bool ShowAll()
{
    return Show == Display.All;
}

public bool ShowExpense(Expected expected)
{
    return Show == Display.Expense && expected.EXPENSE;
}

public bool ShowNonExpense(Expected expected)
{
    return Show == Display.NonExpense && !expected.EXPENSE;
}

Then you can put the expression back into the if-statement:

if (ShowAll() || ShowExpense(expected) || ShowNonExpense(expected))
{
    // code
}

This should be easier to read, and change later on.

+1. Only change I would suggest is to make everything a property (assuming that 'expected' is member of the same class) — SolutionYogi, Apr 01 '13 at 21:40

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

5

Use if statements and extract complex conditions into methods, e.g

if (ShowAll() || ShowExpense())
{
}

Remember about OOP and polymorphism every time you write such 'switch', adding another case to that code will be a nightmare

see this and similar (C++) instructions about converting switches

P.S if you are interested in making your code clean and readable, consider reading Smalltalk Best Practice Patterns by Kent Beck and/or Clean Code by Uncle Bob I really enjoyed both of them, highly recommend.

edited May 23 '17 at 12:09

Community

1
1

answered Apr 01 '13 at 11:37

illegal-immigrant

8,089
9
51
84

Isn't adding more methods going to make the code bigger -> more complex? – AngelicCore Apr 01 '13 at 12:54
1

It will make code bigger, yes. More complex? I doubt that. But surely easier to support and maintain in the future. Make you methods small, arrange them one by one so they are ordered in natural way (method which is called from current is right below) and you will be able to read code like a newspaper – illegal-immigrant Apr 01 '13 at 12:57

score 3 · Answer 4 · answered Apr 01 '13 at 11:34

3

If you want readability, just throw away your syntax trash:

if (Show == Display.All || expected.EXPENSE && Show == Display.Expense || !expected.EXPENSE && Show == Display.NonExpense)
{
    //Code
}

answered Apr 01 '13 at 11:34

Dennis

37,026
10
82
150

2

Agreed. Switch adds nothing for this. – Matthew Watson Apr 01 '13 at 11:34

Hossein Narimani Rad · Answer 5 · 2013-04-01T11:40:17.967

2

Provide the else part for each of them so it will not throw error, however as others say, you actually don't need switch in this case.

switch (Show)
{
    case Display.Expense:
         if (expected.EXPENSE != true)
             // do what you want
             break;
         else 
             // do what you want
             break;
    case Display.NonExpense:
         if (expected.EXPENSE == true)
             // do what you want
             break;
         else 
             // do what you want
             break;
    case Display.All:
        //Code
        break;
}

edited Apr 01 '13 at 11:40

answered Apr 01 '13 at 11:33

Hossein Narimani Rad

31,361
18
86
116

@DavinTryon I agree, but I want to show what is causing the error. any way I edit it. – Hossein Narimani Rad Apr 01 '13 at 11:42

score 1 · Answer 6 · answered Apr 01 '13 at 11:43

The reason why you get this error is that you are not defining break statements.

You defined the break conditionally.

            switch (Show)
            {
                case Display.Expense:
                    if (expected.EXPENSE != true)
                        break;

                // Note that the break above is in scope of you if statement, and will
                // result in a compiler error
                case Display.NonExpense:
                    ...
            }

Either make sure every case statement has its own break or group the case statements as follows.

            switch (Show)
            {
                case Display.Expense:
                case Display.All:
                    // do stuff
                    // Expense and All have the same behavior
            }

score 0 · Answer 7 · answered Apr 01 '13 at 11:39

Refactor out the if statements so you can express it like so:

if (isDisplayAll() || isExpense(expected) || isNonExpense(expected))
{
    // Code
}

The extracted logic:

private bool isDisplayAll()
{
    return (Show == Display.All);
}

private bool IsExpense(Expected expected)
{
    return expected.EXPENSE && (Show == Display.Expense);
}


private bool IsNonExpense(Expected expected)
{
    return !expected.EXPENSE && (Show == Display.NonExpense);
}

score 0 · Answer 8 · answered Apr 01 '13 at 11:47

0

Agree with Dennis, you don't want a switch case for this problem.

Although probably less readable, you can also use shorter:

if (Show == Display.All || (expected.EXPENSE == (Show == Display.Expense)))
{
    //Code
}

answered Apr 01 '13 at 11:47

publicgk

3,170
1
26
45

If Condition inside switch case

8 Answers8