C# – What’s wrong with this code? – #1


Many months ago, I did a TechEd talk entitled “What’s wrong with this code?”. I’ve decided to re-target those snippets into blog posts. If you were at the talk, this may be boring, though I’ll try to add a little anecdotal information in my answers.

Here’s the plan:

  1. I post the snippet
  2. You comment on what’s wrong with it
  3. I post my answer, and any comments that I like
  4. Repeat until I run out of snippetts.

Here’s the snippet:

switch (s) {
   case “a”:
      try {
         HitA();
      }
      catch (Exception e) {
         throw e;
      }
      break;
   case “b”:
      try {
         HitB();
      }
      catch (Exception e) {
         throw e;
      }
      break;
}
 
What’s wrong with this code?
Comments (42)

  1. Brendan says:

    Caught exceptions are automatically thrown, negating their usefulness. Same outcome could be achieved by removing the surrounding try/catch blocks.

  2. JD says:

    I’m sure I’m missing something else, but if you’re catching an exception to do nothing (possibly just to insert a breakpoint) you should use the "throw;" syntax rather than "throw e;" else you’ll blow your stack trace away.

    Otherwise if you’re just sticking do-nothing catch blocks there for no reason, just get rid of them.

  3. Greg says:

    IMO the try/catch blocks serve no purpose other than to make the snippet less readable. So I agree with Brendan, remove the try/catch blocks.

  4. James says:

    Is the ‘break;’ neccessary?

  5. David Grant says:

    Opening curly braces are on the same line as the contexts they’re opening.

    switch (s) {

  6. Mike Weller says:

    Do you need to have a default case? Of course this depends on what happens after the switch, so I can’t see this being the ‘thing’ that’s wrong here.

  7. Mike Weller says:

    Curly brace placement is matter of taste though, it doesn’t change the code

  8. Jakeypoo says:

    I’m betting on the lack of a default statement.

  9. Matthew W. Jackson says:

    Since the exception (mis)handling has already been mentioned, I’ll take another approach. Call me a premature optimizer, but if you know for a fact that a string will only be one character, it would be much better to switch on s[0]. That’s not to say that the C# compiler doesn’t do an excellent job at a string switch. It’s just that C# and the JIT do a *more* excellent job at an integer switch.

    This also begs the question of whether a string should have been used in the first place. Many ex-VB programmers who have moved to .NET do not realize that System.Character exists. Now, if the "single character" might contain surrogate pairs, System.Character would not suffice, but that is not discernable from the posted code.

  10. Vic Berggren says:

    it never fires… if I do this

    public static void HitA()

    {

    Console.WriteLine("hit a");

    }

    public static void HitB()

    {

    Console.WriteLine("hit b");

    }

    static void Main(string[] args)

    {

    string s = "c";

    switch (s)

    {

    case "a":

    try

    {

    HitA();

    }

    catch (Exception e)

    {

    throw e;

    }

    break;

    case "b":

    try

    {

    HitB();

    }

    catch (Exception e)

    {

    throw e;

    }

    break;

    }

    Console.WriteLine("done");

    }

  11. Anders Molin says:

    The formatting 😉

    The curly brakes should go on next line instead of being on the same…

    As in

    try

    {

    instead of try {

    😉

    Ok, I agree with most others, no need for the try catch when they does nothing…

  12. ShadowChaser says:

    #1 – "Exception" generally should not be handled.

    #2 – The exception is being rethrown – no point to catching it, but I am assuming other code will go into that catch handler 😉

    #3 – The try catch block can be wrapped around the switch, which doesnt require that the code be copied & pasted like that

    #4 – "Throw" is a better command than "Throw e". "Throw;" is basically "Rethrow". However, the debugger does not treat it as such 🙁

    #5 – Switching a string is inefficient, there may be other methods that are faster. Enumerations, hashes, etc.

    #6 – I was under the impression you needed { } braces around each "case" statement. But maybe I am just picky and never tried it another way 😉

  13. Jeff Parker says:

    OK, without know more about the piece of code as everyone seems to mention, one way if you look at it it is fine, if it is doing what you intend it do do an assuming you have already cleaned up the input of it, this could be very legitimate code.

    So number one thing wrong but then again may not be, variable s, is this case insensitive? Or not? if it is then you should have a .ToLower() on it. It should also be Trimed. The other thing is it is not culture neutral, I mean there are a lot of things that the variable s could be, it is going to really depend on what all is implements to control what variable s really is.

    The throw thing is no real biggy I mean it will work, but again without knowing things about the methods HitA and HitB hard to tell, Suppose HitA or HitB throw ApplicationExceptions, Then you would want to catch and handle those apropriately before going to the catch all Exception. Maybe you do not need to rethrow them. Syntactically throw e;

    and just throw; is fine and will return the same thing. You could really shorten it up by making the catch

    catch

    {

    throw;

    }

    You could speed it up by using a return in the try block as well, you would have to remove the break statement because as long as you are returning something everywhere the break is not needed and compiler will warn you about unreachable code.

    For Example

    case "a":

    try {

    HitA();

    return;

    }

    catch (Exception e) {

    throw e;

    }

    case "b":

    ……….

    So basically I am not seeing anything wrong with this code if you considered all the other things elsewhere. but you should double check them here you never know when something may be called. And by Who and for what purpose

  14. Bill Sempf says:

    I would bet on the lack of a default option, but I have another thought. Why put try statements inside the Case statements? I would just wrap the whole switch statement in one try block. Or is that silly?

    S

  15. Mike Weller says:

    The guy up there said it doesn’t fire. I can only assume if he is correct (cba to try it) then try blocks aren’t allowed in case statements.

  16. Gabe A says:

    My problem with the code is modularity. The functionality of the switch can be handled within a single method which would decrease the number of methods you need along with the number points the exception is "bubbled up". I agree with the others that a char variable should be used along with a "default". Also, if there are any file or network access code these should be closed within a "finally".

    Best regards,

    Gabe

    private void HitIt(char s)

    {

    switch(s)

    {

    case ‘a’

    try

    {

    // do something

    }

    catch{throw;}

    break;

    case ‘b’

    try

    {

    // do something

    }

    catch{throw;}

    break;

    default:

    throw new Exception("unhandled");

    break;

    }

    }

  17. ben says:

    i think the point of these ‘whats wrong with this code’ pieces is not intended to be stylistic or concerned with best practices, I think it usually has to do with the code not executing correctly. Sure, the ‘throw e’ statements may not be the best way to handle the exception, but that doesnt make for a very interesting blog post. same with moving the try-catch outside the switch, performing a switch on a char instead of a string, and closing file or network resources. The best answer out there is from Vic who actually tried the code out and says that it doesnt work as expected. So I hope eric has a more interesting answer for us.

  18. Roo says:

    By the way, I beleive that a string switch is almost as efficient as an int switch since strings are immutable in the .NET Framework.

    I have tested the snippet and it works like a charm… I just beleive that the try/catch statements should be placed outside the switch for more readability as said earlier.

  19. Gabe A says:

    Ben, I don’t need to try out the code to know that passing "c" to a switch that doesn’t have a case for "c" or a "default" will not process it.

    As a general practice I always use default, but I still have a "style" issue with methods "HitA", "HitB" … "HitZ". But I’m being meticulous.

    Gabe

  20. RichB says:

    string switch is only as efficient as int (or char) switch if the strings are interned.

  21. Haacked says:

    Just using the "throw" keyword is better than "throw e" because the second case doesn’t retain the stack trace information and you’re not adding any useful information.

    If you meant to throw an exception in a catch block that adds information, you should wrap the current exception like so:

    try

    {

    HitA();

    }

    catch (Exception e)

    {

    throw new BetterException("This has more info", e);

    }

    So that the code that catches this information has access to the original stacktrace that caused the exception.

  22. Anon says:

    Apologies if I’m wrong – I usually do VB.NET…

    If you did want to do exception handling, why not move it outside the switch rather than repeat it for every case (particularly relevant if you have many case clauses)…

  23. Besides being 20 lines of code that could be written in 1…

    The try..catch blocks are pointless, and "throw e;" is counterproductive (because the call stack is lost).

  24. The compiler will "bitch" that undetected code has been found.

    This is because when you catch the exception and throw it back, you are bypassing the break.

    Other than that, everything else is "legal". Whether it is prudent is another matter.

  25. Nat says:

    switch statement with string will always intern strings (both the string in switch and case) – Check Jeff Richter’s book for more info. Therefore, the performance will be closed to switch with int. However, I think that JIT engine might provide additional performance optimization for int to do something like binary search instead. I think Eric should be able to provide more insight about that.

  26. Nat says:

    If we try to fix one by one at a time

    1. throw e; definitely doesn’t make sense… change it to throw; would make more sense since it will give the correct stack trace.

    2. however, in doing so, the compiler will give warnings as e is not used anywhere…. so change catch(Exception e) to catch or catch(Exception) would do the trick

    3. nonetheless, try-catch blocks are useless as we just try and catch and throw…. so just remove both try-catch blocks completely 🙂

  27. can’t switch on a string, must use a char:

    switch (a) {

    case ‘a’:

    break;

    case ‘b’:

    break;

    }

  28. Matthew W. Jackson says:

    Yes, switching on a string compares the interned strings (just look at the IL output). The strings you are comparing against are guaranteed to be interned, as they are literals. But the string you are switching on may or may not be interned (it may be from a file, for example), so a call to String.IsInterned is made so that reference equality can be used.

    But getting an interned version of a string involves looking it up in the table of interned strings. This cannot be as efficient as a char switch, since it is basically a dictionary lookup (whether it’s a tree or a trie or a hashtable is beyond me).

    If the string isn’t interned, IsInterned will return null, and therefore it’s obviously not any of the cases (unless you have a null case, I suppose), so the code falls through to the default case.

    What if your input is ‘malicious’ and sends HUGE strings (several kilobytes long)? How efficient is the IsInterned in that case? Maybe if the compiler compared string length first before looking for equality, this wouldn’t be an issue.

    And correct me if I’m wrong, but it seems that the Whidbey C# compiler omits the call to IsInterned for string switches. Or at least it did in this case.

  29. Thomas Eyde says:

    I’ll second the break statement. Why is it really necessary? Haven’t you guys got enough feedback to acknowledge that wasn’t the brightest move? You say it is there to please the C++ developers, because they are your main target. Well, that’s just insulting in a number of ways:

    1. The C++ developers thought they were smart. You just stated they aren’t, as you don’t believe they can adjust to the new rules.

    2. You insult all the other programmers, as they are not the target. At least not more than a minor target.

    3. Finally, you insult everyone not on the C# team, as you think you are smarter than us. You could just have made the break statement optional.

    There is no special error handling, so that could be skipped all together.

    If you still insist on error handling, you could move the whole switch statement inside one try block.

    And since there are no special error handling, but a rethrow, a single, standalone throw should be used. No arguments.

    A single try..catch should anyway be used for the whole switch. If a specialized error handling is needed, each of the called methods should handle that.

    Some are pointing out the missing default, but what if there are no default?

    It could be the switch on string is not efficient, but why should I care? Isn’t this something the compiler should handle for me?

  30. MikeH says:

    Someone said the code doesn’t run as expected. I’ve run it and it does exactly what I expected.

    I therefore think that the worst thing about the snippet, given that it is taken completely out of context, is that catching and throwing the exception loses the stack trace.

  31. Gurri says:

    Being fairly fresh in C#, I think my best

    shot is that you should move out the try

    statement out of the switch statement and

    simply remove the throw statement since it

    throws a catched exception.

  32. AT says:

    What’s wrong? There are 18 lines of source code without any line of comments.

    Without comments we cannot figure out that’s wrong with it. We know nothing about purpose of these sources.

    This can be some very tricky source code for compiler/runtime testcase.

    Or there is some magic involved in how developer willing to see exception stacktraces.

    There are a lot of different ways to fix this source.

    Use try/finally, move catch block out on top of switch or inside HitA/B, provide default case block, wrap exception into another more specific.

    But all those corrections can be wrong.

    This can be a source that developer not yet finished to code, or developer has added those catch blocks to add a logging, or this source code for some mysterious non-CLR complaint framework there some exceptions can be not inherited from Exception 😉

    We know nothing about developer intentions.

  33. David Levine says:

    Possible flaws (almost everyone has it right).

    1. try-catch formulation is poor. Either…

    1. add context information and throw new Exception("context",ex); or…

    2. Add specific error handling to each catch and then use throw, not throw e;

    3. Move try-catch outside switch and handle with common error handling code, and then either use throw new Exception(ex,"context"); or throw.

    4. Eliminate try-catch altogether if there is no error handling code and no context information to add.

    2. switch considerations: we are all assuming that the intent is to compare strings, and not some other data type. The only way to get it to compile is to declare ‘s’ to be a string, but there’s no way to be sure.

    Also, assuming it is a string, no test is made for a null string or a (default) case to handle it. The code as shown is case sensitive, but we don’t know if this is correct. As AT pointed out, without comments or better names in the code it isn’t possible to determine the intent, so it’s not possible to really tell if it is correct or not.

    The performance considerations are also relevant.

    3. Input validation: there is none shown in snippet. is this correct?

    3A. There’s no default case, so there’s no way to determine if the value of s is always constrained to be "a" or "b", or if not, then if not executing a default statement is correct.

    4. Testing – where’s the unit testing that would tell us what the correct output is supposed to be? 🙂

    In other words, the code may behave as intended but there is no way to determine that from the snippet.

    I think this illustrates that providing a snippet without sufficient context to properly analyze it is also a bug 🙂

  34. Daniel Jin says:

    > Nat:

    > However, I think that JIT engine might provide additional performance optimization for int to do something like binary search instead.

    not the JIT, the C# compiler will do a jump table on a continuous block of integer cases. make the switch very fast.

  35. Daniel Jin says:

    > Thomas Eyde

    > I’ll second the break statement. Why is it really necessary? Haven’t you guys got enough feedback to acknowledge that wasn’t the brightest move?

    in some situations, it’s needed to distinguish between a case that is nop from a case that should fall through to the next one.

  36. Eric, not sure, but how about:

    1) Violates "Throw low, catch high"… no need to try catch in the first place….

    case "a":

    {

    HitA();

    break;

    }

    2) No "default:" case ?

  37. Eric, not sure, but how about:

    1) Violates "Throw low, catch high"… no need to try catch in the first place….

    case "a":

    {

    HitA();

    break;

    }

    2) No "default:" case ?

  38. Val Savvateev says:

    1. Rethrowing exceptions this way makes them lose stack information (it gets reset to the line from which the exception is thrown)

    2. Rethrowing expeptions this way is pointless – you don’t add any information into exception, nor do you handle it in any way. Basically in this example there’s no reason to have a try/catch block at all (except for maybe debugging, but that can be done more elegantly – use #if statement)

    3. If the #1 and #2 are not enough and we really want to have the try/catch – put it around switch statement:

    try {

    switch (…) {…}

    } catch (Exception ex) {



    }

  39. AT says:

    Additional problem:

    HitA()/HitB() can return objects that implement IDisposable contract and they must be disposed ASAP 😉

    We know nothing about thouse methods 😉

  40. Could you specify what the ETA is on step 3. ?

  41. trinitron says:

    Instead of switching the try’s you should try the switch.