Should I leave an unreachable break in a case where I throw an exception?

Question

Is it silly of me to leave unreachable break statements in a case that just throws an Exception anyway? The defensive part of me wants to leave it there in the event that the logic changes. Another part of me doesn't want other developers seeing compiler warnings on my code ("Unreachable code detected").

switch (someInt)
{
    case 1:
        // Do something
        break;
    case 2:
        // Do something else
        break;
    case 3:
        // Oh, we don't use threes here!
        throw new Exception("Business rules say don't use 3 anymore");
        break; // Unreachable...until the fickle business rules change...
    default:
        throw new Exception("Some default exception");
        break; // Unreachable...until...well, you get the idea.
}

What to do?

UPDATE

I see a few responses saying that removing the throw at a later date would cause a compiler error. However, simply removing (or commenting) the throw without a break following it would stack the cases, which may be unintended behavior. I'm not saying it's a likely scenario, but...well, is defensive programming about combating only likely scenarios?

Cesura

The break is no longer needed, I remove them because I build with warnings as errors. – Inapproachable

Interesting question - my initial reaction would be to include the break statement, but I think that'd be purely down to habit. – Coreen

pragma warning disable if you feel your concern is valid but don't want to inconvenience others, but being overly defensive can quickly clutter your code base – Beseech

Answer 1

I'd remove them. Several reasons:

• You don't need them at the moment
• Seeing lots of warnings always makes me nervous as you can lose real warnings in the noise coming from this type warning (assuming you have warn as error off).

Holstein

How would you miss "real errors?" Either your code won't compile or you've already decided that a warning isn't a "real error" (i.e. you don't build with the "treat warnings as errors" flag). – Beseech

If you've got 150 warning generated above which are noise and you introduce a something that generates a warning that is useful, it will be easy to miss. – Holstein

Answer 2

I wouldn't "hide" it in the switch. I would throw ArgumentExceptions as soon as possible. That avoids side-effects and is also more transparent.

Somebody might add code before the switch at some point which uses someInt although it is 3.

For example:

public void SomeMethod(int someInt)
{
    if (someInt == 3)
        throw new ArgumentException("someInt must not be 3", "someInt");

    switch (someInt)
    {
        // ...
    }
}

Atheroma

Answer 3

By ignoring some compiler warnings you are reinforcing the bad behavior of ignoring any compiler warning. In my opinion, that's a greater risk than any advantage you gain from leaving the break statements in.

EDIT: I removed my original point about the compiler forcing you to put the break statements back in if the throw statements were to be removed. As payo pointed out, in some cases, the compiler wouldn't. It would just combine the case with the one below it.

Koller

The compiler does not force you to add the break if the case has no other code (after commenting the throw). – Gehlbach

You're right. In that case, the onus is on the programmer to write good code. – Koller

Answer 4

Personally, I would never leave unreachable code in production code. It's fine for testing, but don't leave it as such.

You would never do this would you?

public void MyMethodThatThrows()
{
    throw new Exception();
    return;  // unneeded
}

so why keep the break?

Blythe

I understand the first part of this well and it's a good point. The example, however, is somewhat of a straw man because while I wouldn't put a return inside a void, I certainly would put breaks inside of a switch. Of course this question is about breaks rather than returns in a switch because, well, I'm a one exit kind of guy. – Cesura

@TimLehner, ha, good point. my example was basically just to over exaggerate the issue. – Blythe

Answer 5

I'd remove it.

It gets rid of the warnings, and even if the logic were to change and the Exception to be removed, you'd receive a compiler error saying that a break needs to be added back in.

Zacharyzacherie

Actually there is a case where we are both wrong. If the exception is removed and nothing added the body will be empty and allow for fallthrough without warning or error. – Halliday

Answer 6

The defensive part of me wants to leave it there in the event that the logic changes.

What logic exactly is subject to change here? What happens when an exception is thrown is documented pretty well. And I think you can be reasonably sure that no later revision to C# will effect a breaking change of "all programs written so far are no longer working" magnitude.

Even if there were no compiler warning, redundant code is not a good thing unless there is the possibility that the code might come into play by preventing bugs during routine maintenance, which in this case we cannot reasonably say that such a possibility exists.

Crystacrystal

I think he means if the exception is removed, but even then it isn't a problem as you have to have one or the other. – Halliday

Perhaps I used the wrong terminology there, but no, I don't mean if the C# specification changes, only fickle business rules. – Cesura

Answer 7

The defensive part of me wants to leave it there in the event that the logic changes.

Well, if you remove the throw and forget to add a break the compiler will let you know.

I see no reason for the break to be there. It is redundant and you will just trigger a warning anyway, so I would remove it. There is no good reason to leave useless code around cluttering up your logic.

Halliday

Simply removing the throw w/o a break there would stack the cases, which may be unintended behavior. – Cesura

@TimLehner: That's true since the body would be empty. That said... if you left the body empty then I assume you did so for a reason. I would hope that you could trust your coworkers to know the basics of how a switch works. – Halliday

Don't we all hope for the same things, really? :) – Cesura

@TimLehner: Hehe, yeah, but honestly... mistakes will happen, there is no idiot proof option here. If you have a developer who constantly makes bone headed mistakes... time to find a new staff member. – Halliday

break is not required on an empty case, which would be the case if the throw is removed. – Gehlbach

@payo: Thank you for reiterating that. – Halliday

Answer 8

I wish my project sources were so cool and trouble-less, that I'd have to think about such minor cases of defensive programming :)

I mean there are so-o-o many pitfalls in software-development - COM and Interop, Security/Permissions and Auth, Certificates, that ... gosh, can't tell you how much code I'd have to write to defend myself from shooting in the foot.

Just remove this break, because it's redundant, it's confusing for fellows who knows that 'case' should end with break or return and should get being obvious for fellows that don't know this.

Remunerative

True, this is nobody's biggest problem in life. Now, case ending in return is definitely up for debate... – Cesura

Answer 9

MSDN C# Reference states: "A jump statement such as a break is required after each case block, including the last block whether it is a case statement or a default statement."

So, doesn't "throw" constitute a "jump statement"? I think it does. Thus, "break" is not required, and the "unreachable" issue goes away. :)

Aileenailene

The OP acknowledged that the breaks are not necessary, but is considering leaving them in defensively. Consider what happens if you decide to do something other than throw an exception. – Gavelkind