7

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?

Tim Lehner
  • 14,813
  • 4
  • 59
  • 76
  • 1
    The `break` is no longer needed, I remove them because I build with warnings as errors. – asawyer Mar 29 '12 at 21:56
  • 1
    Interesting question - my initial reaction would be to include the break statement, but I think that'd be purely down to habit. – KazR Mar 29 '12 at 21:59
  • 1
    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 – Michael Mar 29 '12 at 22:09

9 Answers9

7

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).
FishBasketGordo
  • 22,904
  • 4
  • 58
  • 91
gbanfill
  • 2,216
  • 14
  • 21
  • 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). – Michael Mar 29 '12 at 22:02
  • 1
    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. – gbanfill Mar 29 '12 at 22:04
7

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)
    {
        // ...
    }
}
Tim Schmelter
  • 450,073
  • 74
  • 686
  • 939
2

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.

FishBasketGordo
  • 22,904
  • 4
  • 58
  • 91
2

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?

jb.
  • 9,921
  • 12
  • 54
  • 90
  • 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. – Tim Lehner Mar 30 '12 at 04:26
  • @TimLehner, ha, good point. my example was basically just to over exaggerate the issue. – jb. Mar 30 '12 at 05:22
1

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.

Brandon
  • 68,708
  • 30
  • 194
  • 223
  • 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. – Ed S. Mar 29 '12 at 22:08
0

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

jerry
  • 2,581
  • 1
  • 21
  • 32
Matthew A. Schneider
  • 1
  • The OP acknowledged that the `break`s are not necessary, but is considering leaving them in defensively. Consider what happens if you decide to do something other than throw an exception. – jerry May 03 '13 at 21:31
0

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.

Jon
  • 428,835
  • 81
  • 738
  • 806
  • 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. – Ed S. Mar 29 '12 at 21:57
  • Perhaps I used the wrong terminology there, but no, I don't mean if the C# specification changes, only fickle business rules. – Tim Lehner Mar 29 '12 at 22:10
0

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.

Ed S.
  • 122,712
  • 22
  • 185
  • 265
  • 2
    Simply removing the throw w/o a break there would stack the cases, which may be unintended behavior. – Tim Lehner Mar 29 '12 at 22:03
  • @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. – Ed S. Mar 29 '12 at 22:07
  • Don't we all hope for the same things, really? :) – Tim Lehner Mar 29 '12 at 22:08
  • @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. – Ed S. Mar 29 '12 at 22:09
  • break is not required on an empty case, which would be the case if the throw is removed. – payo Mar 29 '12 at 22:28
  • 1
    @payo: Thank you for reiterating that. – Ed S. Mar 29 '12 at 22:28
0

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.

alex.b
  • 4,547
  • 1
  • 31
  • 52