As time goes by, things go wrong with any body of code: we get lazy, bad developers contribute code, the original clean architecture gets forgotten, and so forth. Some bad code is easy to spot: it simply “smells” - once you see it, you know it’s bad.
Refactoring this sort of code to remove the smell isn’t often that difficult - and the time taken will be repaid next time you have to make modifications.
The following are my ten worst “odors” in code:
1. Long Functions
Over time functions often get longer and longer - extra functionality is added, edge cases are handled, and so forth. However, long functions are difficult to change, and difficult to understand.
Each function should ideally do exactly one thing: you should be able to sum it up in one sentence (and the method name should be a summary of that sentence).
If you find a function is getting too long, split it into sensible parts. Each part should follow the above rule.
2. Commented-Out Code
Some of these rules are open for debate. Not this one. I’ll go so far as to say that there is never an excuse for commenting out code.
- Are you trying to preserve some sort of history to the code? You should be using source control.
- You’re not sure if the code might be necessary: find out - and don’t commit until you’re sure.
- Perhaps you’re in the process of shotgun-debugging: step away from the code, and work out exactly what’s going on.
3. Copy-Pasted Code
Like commented out code, there’s really no excuse here. Programming IDEs should only support ‘cut’ and ‘paste’ - there’s no need for ‘copy’.
Instead of copying code, refactor it into a method, and call it. Copy-pasting is lazy - period.
4. Handled-but-Really-Unhandled Exceptions
I need to explain here: I’m not suggesting that every function should handle all exceptions that could be thrown. Rather, I think the handling of exceptions falls into two categories:
- The function can handle a particular exception, and takes some action to correct it.
- The function cannot handle a particular exception, and does not attempt to catch it.
What I’m referring to is code like this:
void Foo()
{
try
{
// ...
}
catch( Exception e )
{
// Throw exception away, or just log it.
}
}
In general, doing nothing with an exception is a mark of bad code. If the function can’t handle it, then it shouldn’t be caught. Alternatively, if it didn’t need to be thrown, then try to amend the throwing code.
(Especially bad is throwing and catching the same exception in a single function. Exceptions should not be used for flow control; in situations like this, the entire function needs to be re-thought).
5. Large Numbers of Parameters
Functions taking large numbers of parameters (say, more than half a dozen) are usually a bad idea, and are generally indicative of either a function trying to do too much, or poor class organisation.
Often a function will grow to accommodate new functionality, and hence grows parameters. Split the function into separate, smaller, simpler functions.
Alternatively, if the function absolutely cannot be split, consider introducing a new class which encapsulates some or all of the original parameters, and passing that class as an indirect parameter.
6. Non-Obvious Names
Variable or function names such as foo or bar, swearwords, names, or otherwise clever or amusing really don’t have a place in professional code. They might make you laugh, but when someone else tries to read your code, they won’t thank you. (And remember - that ‘other’ person reading your supposedly clever code might be yourself in six months’ time).
The use of i,j,k is prrobably a little more open to debate - however their use as loop indices can usually be avoided if your language or libraries support more generic iteration constructs.
7. Deep Nesting
Just as functions shouldn’t be too long, so they shouldn’t be too wide. There’s no hard-and-fast rule, but if your conditionals or loops are nested more than about three deep, you should consider refactoring your code. Either pull the inner parts of the code into separate methods, or pull the complex conditions into functions. A line of code should never exceed approximately seventy characters (the standard editor width).
8. Beacon Comments
Sometimes you don’t even need to smell bad code - the original developer has helpfully pointed it out in the comments! If you see comments like:
// This is hacky...
// TODO: This is bad. FIX IT!
// This code ought to be refactored
…then you know something needs to be done.
9. Narcolepsy
…by which I mean inappropriate sleep() ing. If your code requires an (essentially random) sleep interval to function correctly, it’s likely to fail if the user’s machine is especially old, especially new, or just busy. Try attaching to an event that signals when the situation you want to be in occurs.
Another related smell is relying on the speed of the computer, the speed of video frames, or similar, to achieve a particular timing effect.
10. Helper Classes
This is something I’m guilty of from time to time, and I’m looking for comments as to the best way to avoid it. Helper or utility classes always seem to grow in large bodies of code, usually containing unrelated static methods. These clumps should be analysed and split or moved into classes that have one task.
via http://www.hackification.com/2009/01/20/sniff-out-that-smelly-code/
Posts
