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
}

Solution 1

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.

Solution 2

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();

Solution 3

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.

Solution 4

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
}

Solution 5

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;
}

View more solutions

Comments

[Matthew Watson]

Agreed. Switch adds nothing for this.

[Hossein Narimani Rad]

@DavinTryon I agree, but I want to show what is causing the error. any way I edit it.

[AngelicCore]

Isn't adding more methods going to make the code bigger -> more complex?

[illegal-immigrant]

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

[SolutionYogi]

+1. Only change I would suggest is to make everything a property (assuming that 'expected' is member of the same class)