TalesFromTheSmellySide<Code> – Episode #1

What is this?

Well, it was inevitable that my long stint of greenfield projects was going
to come to end at some point, especially since I’m in the consulting world.  So
now that I’m starting to find myself working in some pretty scary code bases
again, my
fellow LosTechies dude
suggested I start blogging about my experiences. 

And since I’ve already seen many, many things that make me say “huh?” and, at
times, brings tears to my eyes, I figured I’d give this first post a number
(assuming there are many more coming).

Ok, so really this is just an opportunity for my fellow developers to
perhaps not feel so bad about the code bases they are working in, because maybe,
just maybe, someone else out there has it worse.  (As I’m sure there are folks
who are forced to work in stinkier code than I…)

In order to make these somewhat useful, I’m making them all about the code. 
Smells that I see, and hopefully some simple solutions for fixing them, leading
to more maintainable and readable code.

Episode #1

catch (Exception ex)
{
if (ex.GetBaseException().Message.Contains("You gotta know this just *feels* wrong!"))
{
// do something
}
}

This one definitely had me scratching my head.  I can honestly say this is
the first time I’ve ever seen a conditional expression based on the actual
string message on an exception.   Scary, is it not?

Of course the first way to correct this smell that jumped out at me was just
to simply create a custom exception and catch it in its own catch block.

catch (SomeCustomException ex)
{
// do something
}

 

What kind of smelly exception-related code have you seen and
corrected?

 

Related Articles:

Post Footer automatically generated by Add Post Footer Plugin for wordpress.

This entry was posted in tales from the smelly side. Bookmark the permalink. Follow any comments here with the RSS feed for this post.

6 Responses to TalesFromTheSmellySide<Code> – Episode #1

  1. Sean Scally says:

    I’ve seen this before in a situation that was not totally “wtf”-based. It was in code that was dealing with a web service. Legacy .NET 1.1 and 2.0 web services only can throw one kind of exception, so if you want to include any other sort of information to the client, you have to either use return values or sniff exception strings.

    I’m not saying that was the case here, or that even that particular situation forgives such a practice… :)

  2. Joe Ocampo says:

    I would still leave in the

    catch (Exception ex)
    {

    }

    and stack them, like so:

    try
    {

    }
    catch (SomeCustomException ex)
    {

    }
    catch (Exception ex)
    {

    }

    This way in case I missed an instance where the message is being thrown in won’t break the existing behavior. You can then incrementally refactor towards your typed exception with out worry.

    You should be able to run a usage report on the class and proceed with refactoring accordingly. I am sure I am not telling you anything you didn’t already know but this is for developers that are new to refactoring legacy code.

  3. Joe says:

    Heres the one that kills me, and makes me want to take a hammer to people’s grubby little fingers:

    try
    {



    }
    catch(Exception e)
    {
    //empty block…
    }

  4. Sean Scally says:

    Yeah, I have seen that second one from time to time. When it’s out of ignorance I can forgive it, but when it’s out of laziness I tend to wonder why that person is writing code.

  5. jssingh says:

    @Sean, one of the ways to present meaningful exceptions details to the clients of the web service is to embed the actual exception within SoapException (which is the exceotion that always gets thrown to the client).

    The client would basically iterate through the inner exceptions of the Soap Exception and take action accordingly.

    This explanation does not in any kind justify the mess created by an inexperienced programmer as Joey describes. That is utterly inexcusable.

  6. Sean Scally says:

    @jssingh

    That is true; My problem with it is that it requires some things I don’t like, such as code on both the client and server side to handle the wrapping and unwrapping of the exception in a uniform manner. I can’t always make the assumption that the service is being used by someone that knows how to unwrap the exceptions properly, or even has the capability to do so.

    The real problem is that the WS/SOAP specs don’t allow for robust exception handling, and my understanding is that future versions of MS WebServices/WCF lets you express exception data as part of the service descriptor, which makes this whole ugly mess unnecessary :)

    I sure have gotten far from the topic at hand. Back to smelly code! I may have to write a lengthier response later.