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

Thanks for all the responses. There were many correct ones, and a number that I want to at least comment on. Plus one that will get its own post...

If you've written Win32 code, you've probably written code like:

result = ::CallWin32Function();

if (!result)
    return result;

result = ::CallAnotherWin32Function();

if (!result)
    return result;

The code that I posted is the naive translation of that idiom into C#. Call a function - figure out whether it succeeded - if it didn't, return back to the caller.

What many people pointed out correctly was that the try-catch in this code wasn't doing anything useful, and therefore the answer I was looking for was "the useless try-catch constructs". Several of you said that you should wrap that whole switch in a try-catch. That's certainly nicer than the code I showed, but still had the useless construct, so I won't award any points for that answer.

There was also a comment about lack of comments. While I'm a big fan of algorithmic comments (ie explaning the approach that code takes), I think that code comments are mostly a crutch to explain code that isn't as clear as it could be. Code with well-chosen names and good OO design should require few comments.

There was also a sub-thread on the efficiency of the switch on string. Not only is that not what I was looking for, but without measurement to determine whether it would *ever* matter what implementation you chose, it's wasted time to even talk about it. So knock it off.

Interestingly, I was writing some code this morning where I needed to ensure that items there was only one item selected in a list and then pull the selection out. I already had a (written and well-tested) method that told me how many items were selected, but I had to work hard to not write a complex single loop to do what I needed when calling the method and then writing a simple loop got me there easier. It's definitely not as efficient, but I think it's unlikely I'll ever need it to be faster than it is.

I should also probably have noted that "snippet" implies that the code is pulled out of a larger context, so it's not necessarily going to be fully understandable as a snippet. That narrows the search a fair bit - since you don't know what the code is really supposed to do, you can be assured that the problem has nothing wrong to do with that.

As for the code, it came from some sample code in an early .NET programming book, and there were around 8 diffent cases in the switch.

I'll post another one of these next week.

Comments (3)

  1. David says:

    Of course, the reason I didn’t see anything wrong with the code is because I’ve actually seen requirements that it appear any exceptions thrown must appear to be thrown from a specific function instead of one of the functions that function calls. I agree its a lame idea, but it is occasionally what people want their code to do.

  2. MBA says:

    Helpful For MBA Fans.

Skip to main content