Code Smells
Welcome to the third edition of Code Smells! Periodically I’ll be posting about how to detect code smells and what they mean in terms of the big picture of your code. The previous installment can be found right here.
What’s a code smell? Wikipedia says it perfectly:
In computer programming, code smell is any symptom in the source code of a program that possibly indicates a deeper problem. Code smells are usually not bugs—they are not technically incorrect and don’t currently prevent the program from functioning. Instead, they indicate weaknesses in design that may be slowing down development or increasing the risk of bugs or failures in the future.
These code smells are often based on my own opinion and experience with programming. If you disagree with what I'm saying in my post, please don't hesitate to post a comment. I'd love to clarify anything I may have worded poorly and discuss your perspective--especially if you have a completely different take on things!
The Stink List
Code Smell #7: Using exceptions to control logical flow. This is a pretty nasty path to get into and a bad code smell to stumble upon. Luckily, it's generally relatively easy to improve. Using exception handling to control logical flow is, in general, misleading. It's relying on a mechanism used to catch unexpected errors in order to direct the flow of your program. Oftentimes times we can use things like if statements to check for these conditions before throwing exceptions.
A common scenario where I see this is parsing. I'll illustrate this code smell with a little C# example:
[csharp]
var input = "This is not a number"; try { var parsed = int.Parse(input); DoSuccessfulStuff(parsed); } catch (Exception) { DoFailStuff(); }
[/csharp]
It seems a bit contrived, but I've seen lots of code written like this--and you know what? This works. It gets the job done. However, there are other mechanisms built-in to .NET that let us do parsing a little bit nicer:
[csharp]
var input = "This is not a number"; int parsed;
if (int.TryParse(input, out parsed)) { DoSuccessfulStuff(parsed); } else { DoFailStuff(); }
[/csharp]
The second set of code doesn't need to catch exceptions to know that the parsing wasn't successful. It may not be obvious from this example but throwing and catching exceptions can be quite expensive compared to a few logical sanity checks instead (and before getting into a debate on this, just know the impact it has on your program. I've had things go from taking several seconds to multiple minutes, but there are certainly cases where performance will be negligible).
The additional kicker in my contrived example is using the base Exception class to create what a colleague of mine refers to as Pokemon Exception Handlers. Thus, even if you didn't want to restructure your code, using a specific exception type would:
- Indicate to other programmers what you're trying to accomplish
- Not swallow other potential problems and have them go unseen
Now, this code smell isn't always possible to avoid entirely. If you're interfacing with third party components, sometimes you do have to rely on catching exceptions that you can't otherwise check for ahead of time. If you don't have the code, you can't know for every path how/when/why exceptions will be thrown. The same thing could be said when running within an environment where state cannot be guaranteed. Sometimes it's just necessary. In this case, I would suggest that instead of using Pokemon Exception Handlers, you try to catch the specific exceptions you know you need to watch out for.
Takeaway:
- If a simple logic check can be used instead of throwing/catching exceptions, it's likely a better bet.
- Try to avoid exception handlers that catch all exceptions. Something nasty might sneak by as a result of it.
- Interfacing with some code or working in certain environments means you have to rely on exception logic. Take a deep breath and move on.
Code Smell #8: Having an object hierarchy that requires many very light weight classes. Object oriented programming and how object hierarchies are structured are pretty complicated topics of discussion, so I'm not about to try and over simplify it with discussion of this code smell. This is mostly something I've gathered from my own programming experience, so I'll try to illustrate with examples that parallel things I would have come across.
When I'm building an object hierarchy, sometimes it's not really apparent just how big and complicated it might get. I might start off with a base class and two child classes of it. Over time, the top three levels in my hierarchy have all turned into some sort of class abstraction, and I don't hit concrete implementations until a few levels down the hierarchy. Not a big deal--Sometimes it's just hard to tell where things will go. When things get to the point where in order to introduce a new class and functionality all I need to do is inherit a class and override a single property or method, that's a bit of a red flag for me.
On the surface, this seems pretty cool. The hierarchy is apparently solidified enough that extending it is really simple if all I need is a single property or method replacement. So why is this a code smell?
In my opinion, it has to do with the duplication of code. If I end up having many child classes (where child in this case represents the child-most class of my mature object hierarchy) that differ only by a single property or method, then I should look at how these classes are being constructed. I wrote recently about how I used lambda expressions to refactor similar code with classes that differed by a single method. My solution, in this case, was to examine the factory that created my classes. Instead of having 10's of different child classes with a bunch of boilerplate code, I had one factory that could specify the code that differentiated each class.
The benefit of this?
- Keep class hierarchies from ballooning our of control. Changing an API down the road can mean making changes in many spots.
- Reduce duplication of boilerplate code. This might be the code required to define a simple constructor or override a getter property.
This is only one small example, but if you get into this situation in your class hierarchy, I'd recommend investigating to see if you can refactor in a similar approach. Maybe your class hierarchy is incredibly mature and isn't changing much. If that's the case, you may not even want to touch it. So be it. If you're still actively adding classes to your hierarchy, it may be better to try analyzing it sooner rather than later.
Takeaway:
- Object oriented design is a complex topic of discussion.
- There's no one perfect way to make your class hierarchies.
- Having many child classes that differ by a property/method or two may be worth checking for refactoring opportunities.
Summary
I hope you enjoyed this issue of Code Smells. As always, it'd be great to open the floor to discussion. I don't believe in absolutes, so identifying code smells is not meant to be me preaching some made up laws of programming. Let me know your thoughts on these code smells or share code smells of your own!