Freigeben über

C# - What's wrong with this code? - #1

  • Artikel

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

  • Anonymous
    November 11, 2004
    Caught exceptions are automatically thrown, negating their usefulness. Same outcome could be achieved by removing the surrounding try/catch blocks.
  • Anonymous
    November 11, 2004
    The comment has been removed
  • Anonymous
    November 11, 2004
    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.
  • Anonymous
    November 11, 2004
    Is the 'break;' neccessary?
  • Anonymous
    November 11, 2004
    The comment has been removed
  • Anonymous
    November 11, 2004
    The backets are incorrectly formatted. :P

    I didn't find any difference between throw and throw e in my tests, so that shouldn't be the problem. The try-catch blocks do seem like a waste unless you're debugging the code. The only thing left that I see is there is no default case.
  • Anonymous
    November 11, 2004
    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.
  • Anonymous
    November 11, 2004
    Curly brace placement is matter of taste though, it doesn't change the code
  • Anonymous
    November 11, 2004
    I'm betting on the lack of a default statement.
  • Anonymous
    November 11, 2004
    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.
  • Anonymous
    November 11, 2004
    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");

    }
  • Anonymous
    November 11, 2004
    The comment has been removed
  • Anonymous
    November 11, 2004
    #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 ;-)
  • Anonymous
    November 11, 2004
    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
  • Anonymous
    November 11, 2004

    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
  • Anonymous
    November 11, 2004
    The comment has been removed
  • Anonymous
    November 11, 2004
    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;
    }
    }
  • Anonymous
    November 11, 2004
    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.
  • Anonymous
    November 11, 2004
    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.
  • Anonymous
    November 11, 2004
    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
  • Anonymous
    November 11, 2004
    string switch is only as efficient as int (or char) switch if the strings are interned.
  • Anonymous
    November 11, 2004
    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.
  • Anonymous
    November 11, 2004
    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)...
  • Anonymous
    November 11, 2004
    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).
  • Anonymous
    November 11, 2004
    The comment has been removed
  • Anonymous
    November 11, 2004
    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.
  • Anonymous
    November 11, 2004
    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 :)
  • Anonymous
    November 11, 2004
    can't switch on a string, must use a char:
    switch (a) {
    case 'a':
    break;
    case 'b':
    break;
    }
  • Anonymous
    November 11, 2004
    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.
  • Anonymous
    November 11, 2004
    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?
  • Anonymous
    November 11, 2004
    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.
  • Anonymous
    November 11, 2004
    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.
  • Anonymous
    November 11, 2004
    The comment has been removed
  • Anonymous
    November 11, 2004
    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 :-)
  • Anonymous
    November 11, 2004
    > 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.
  • Anonymous
    November 11, 2004
    > 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.
  • Anonymous
    November 11, 2004
    The comment has been removed
  • Anonymous
    November 11, 2004
    The comment has been removed
  • Anonymous
    November 12, 2004
    The comment has been removed
  • Anonymous
    November 12, 2004
    Additional problem:
    HitA()/HitB() can return objects that implement IDisposable contract and they must be disposed ASAP ;-)
    We know nothing about thouse methods ;-)
  • Anonymous
    November 12, 2004
    Could you specify what the ETA is on step 3. ?
  • Anonymous
    December 05, 2004
    Instead of switching the try's you should try the switch.
  • Anonymous
    June 08, 2009
    PingBack from http://toenailfungusite.info/story.php?id=1431