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

StackOverflow https://stackoverflow.com/questions/9934140

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?

Was it helpful?

Solution

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).

OTHER TIPS

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)
    {
        // ...
    }
}

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?

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.

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.

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.

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.

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.

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. :)

Licensed under: CC-BY-SA with attribution
Not affiliated with StackOverflow
scroll top