Unreachable code detected in case statement

I have a code:

    protected override bool ProcessCmdKey(ref Message msg, Keys keyData)
    {
        switch (keyData)
        {
            case Keys.Alt|Keys.D1:

                if (this._condition1)
                {
                    return true;
                }
                else
                {
                    return base.ProcessCmdKey(ref msg, keyData);
                }

                break;

            case Keys.Control |Keys.U:

                if (this._condition2)
                {
                    return true;
                }
                else
                {
                    return base.ProcessCmdKey(ref msg, keyData);
                }

                break;

            default:

                return base.ProcessCmdKey(ref msg, keyData);
        }

        return true;

It gives me "unreachable code detected" warning on breaks.

Is it good practice not to use break operator here ? I don't want to turn off "unreachable code detected" warning开发者_运维问答.

PS: There are many case in my ProcessCmdKey method.

break is not necessary if all paths in a case statement end with a return. Do not use it then, otherwise you will get the mentioned warning.

There are three unreachable statements in your code, first two are the break statements and the last one int he last line "return true" is also unreachable, I dont know whether C# compiler detects that or not, but logically there is no way last return statement will also be reached.

There are multiple ways to solve this issue,

1. Store a temp variable, called bool retVal, keep retVal and break your switch case and at the end of function return retVal.
2. If you return value before break, break statement is useless.

Better Design Way

If you return values within switch cases, it may be difficult to analyze your code later on by you or someone else, usually it is better to keep a return value temp variable and return it at end of function, that becomes easier to debug and understand the code for new coder.

Switch can be complicated, and more returns within switch may not have better control, if you want to implement logging, debugging, returns from cases could be complicated. And it becomes way to difficult browsing the logic flow graphs.

So it is better to avoid return altogather from case, but still it depends on situtations as well, one needs to make an intelligent decision here.

In this case, good practice imho would be to end each case with a return, like this:

case Keys.Alt|Keys.D1:
    bool result;
    if (this._condition1) 
        { 
            result = true; 
        } 
        else 
        { 
            result = base.ProcessCmdKey(ref msg, keyData); 
        }
    return result;

Or

case Keys.Alt|Keys.D1:
    bool result = (this._condition1)
        ? true
        : base.ProcessCmdKey(ref msg, keyData); 
    return result;

There's nothing wrong with removing the break statement here.

You use return in both conditions. So break could be easily removed.

But I prefer to leave it there in case you will change your code sometime.

As well as removing the break; lines from there, you could also remove the else statements too. It's unneeded since you are returning from the first if.

So your code might look like this instead:

protected override bool ProcessCmdKey(ref Message msg, Keys keyData)
{
    switch (keyData)
    {
        case Keys.Alt|Keys.D1:
            if (this._condition1)
                return true;                

            return base.ProcessCmdKey(ref msg, keyData);

        case Keys.Control |Keys.U:
            if (this._condition2)
                return true;

            return base.ProcessCmdKey(ref msg, keyData);

        default:
            return base.ProcessCmdKey(ref msg, keyData);
    }

    return true;
}

You can slim it down even more by removing your return true; lines and inverting your if statements, because you return true and the end of the method anyway:

protected override bool ProcessCmdKey(ref Message msg, Keys keyData)
{
    switch (keyData)
    {
        case Keys.Alt|Keys.D1:
            if (!this._condition1)
                return base.ProcessCmdKey(ref msg, keyData);

            break;

        case Keys.Control |Keys.U:
            if (!this._condition2)
                return base.ProcessCmdKey(ref msg, keyData);

            break;

        default:
            return base.ProcessCmdKey(ref msg, keyData);
    }

    return true;
}

EDIT: I forgot that you can't fall through using C# so you'd need a break; in each case. Which kind of ruins the nice readability of that block.

Break statement will never be executed, cause you return from the method. So I suggest to remove the unnecessary break.

There is a certain misunderstanding that all case blocks in a C# switch must end with a break. In fact they must end with a jump statement in all code paths; this can be break, return, goto or even continue (or throw, of course). Since the compiler will raise an error if there is a code path with no jump statement, there is absolutely no reason for the final break in the example. The compiler will not let you change your code to a version where the final break would be reached, and this would be the moment to add it.

You could rewrite your code to be a lot shorter and less repatative:

bool ret = false;
switch(keyDate){
    case Keys.Alt | Keys.D1:
         ret = this._condition1;
    break;
    case Keys.Control |Keys.U:
         ret = this._condition2;
    break;
    default: break;
 }
 return ret || base.ProcessCmdKey(ref msg, keyData);

Bitter experience has taught me to always include break statements unless you really mean to fallthrough to the next statement and even then, comment it. Otherwise a function could behave wildly differently because another developer changed something late on a Friday afternoon and didn't see the missing break.

If the function - however large - conforms to the same if...return...else....return structure throughout, you could define a return code variable at the start of the function. Then assign it in your case statement and return it at the end, whatever value it turns out to be.

A switch statement is not necessary here at all, avoiding the problem altogether:

protected override bool ProcessCmdKey(ref Message msg, Keys keyData) 
{ 
    return
        (keyData == Keys.Alt|Keys.D1 && this._condition1) ||
        (keyData == Keys.Control|Keys.U && this._condition2) ||
        base.ProcessCmdKey(ref msg, keyData); 
}

As the code is currently, the "break" commands will never be run nor will the final "return true" command. Removing these would get rid of the warnings.

You might want to try for a solution with fewer return paths as it can make code harder to debug and understand. Something like this:

    protected override bool ProcessCmdKey(ref Message msg, Keys keyData)
    {
        bool processed = false;
        switch (keyData)
        {
            case Keys.Alt | Keys.D1:

                if (this._condition1)
                    processed = true;

                break;

            case Keys.Control | Keys.U:

                if (this._condition2)
                    processed = true;

                break;
        }

        if (!processed)
            processed = base.ProcessCmdKey(ref msg, keyData);

        return processed;
    }

I understand that this doesn't answer the question directly, but inspired by the various answers here I just wanted to add another variation on how the "switch" could be structured:

protected override bool ProcessCmdKey(ref Message msg, Keys keyData)
{
    if (keyData == Keys.Alt|Keys.D1 && _condition1)
        return true;

    if (keyData == Keys.Control|Keys.U && _condition2)
        return true;

    // ...repeat for other variations

    return base.ProcessCmdKey(ref msg, keyData);
}

According to your code, all the break(s) and the last statement are never reached, for there are the return statements before.

You could rewrite your code like this:

    switch (keyData)
    {
        case Keys.Alt|Keys.D1:
            if (this._condition1) return true;
            else goto default;

        case Keys.Control |Keys.U:
            if (this._condition2) return true;
            else goto default;

        default:
            return base.ProcessCmdKey(ref msg, keyData);
    }