Greetings to you truth-seekers, refactorites, and code monkeys all across the digital plane. I am here, El MoffDo, your highly-trained web publication specialist, on the Wednesday edition of I Built His Cage, ready for another post of Programming Excellence.
1. Long Functions
2. Commented-Out Code
3. Copy-Pasted Code
4. Handled-but-Really-Unhandled Exceptions
5. Large Numbers of Parameters
6. Non-Obvious Names
7. Deep Nesting
8. Beacon Comments
9. Narcolepsy …by which I mean inappropriate sleep() ing.
10. Helper Classes
Don’t you see what’s happening here? It’s attached itself to me!
I am ready to accept your apologies for doubting me
Ladies and gentlemen, I don a custom-fitted gas mask every time I step behind the standard-issue EIP keyboard, for I must deal with 9 out of 10 of these smells every day. I’m sure the only thing keeping it from a perfect 10 out of 10 is the fact that there is very little multi-threading going on in our apps.
On commented-out code:
You’re not sure if the code might be necessary: find out – and don’t commit until you’re sure.
Thanks to deadlines, we don’t always have this luxury. And thanks to how there is always too many ways to achieve the same goal, I end up leaving commented-out alternatives in some of my commits. Sue me. If I come back to the code later, I don’t want to run in circles again wondering why I did something the way I did. That sort of nonsense is reserved exclusively for this blog.
How else will I know what other ways I considered and passed up?
On handling every exception:
What I’m referring to is code like this:
catch( Exception e )
// Throw exception away, or just log it.
Eerie. Way too eerie. I see code like this all over the place. Somebody somewhere in an ivory tower decided that grepping through heaps of log files when a production issue comes up is vastly superior to letting things fall apart explicitly, which they did anyway — hence the production issue.
Yet when I want to create an exception class for truly exceptional cases, like EmptyBuildingException, I get shot down because we don’t have enough room on the heap to instantiate another object.
Hello? If you are that low on heap space, you’re doing it wrong.
On 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.
No kidding. My code is littered with vicious cheap shots at myself. I have no other recourse when I’m forced to write procedural Java. I want my name cleared for posterity.
Do NOT doubt me
While I’m at it, I present you my list of objectionable odors:
- God Class: probably my #1 complaint. It is part of the backwards ideology that some developers I work with have, and that is that writing a new class is somehow a very taxing activity, and having to instantiate that new class is even worse. As I alluded to before, if you’re worried about heap space, you’re doing it wrong.
Another of my favorites is “all the code that involves X should be in one class so it is easy to find.” Sigh.
Did you ever stop to think how you’d violate the architecture that way? If multiple teams need to use X, that will never work.
“All you have to do is use reflection.” Brilliant segue into the next smell:
- Reflection Obsession: as far as I know, this is not an official smell, so I’m officially coining the term. This is when the reflective features of your language are used with impunity to
- avoid architectural dependency violations
- eliminate “similar” code
- form the basis of some sort of generic business logic functionality
The resulting code is an unreadable pile of garbage. To fix this smell, use any other programming technique you can think of.
- Type System Subversion: going hand-in-hand with Reflection Obsession is Type System Subversion. In a statically-typed language, this involves declaring everything as type Object, or its equivalent. In a dynamically-typed language, this involves Hungarian notation and/or enforcement of the types of your objects at run-time.
To fix this in statically-typed languages, use interfaces.
- Long Method: this one isn’t as bad as the first two, but I recently had to split up a method at work because it wasn’t giving us the functionality we needed because it was doing two things. What is worse is that the second thing it was doing wasn’t reflected in the name of the method.
Now that I think about it, this smell is actually pretty bad, because the idiotic counter-arguments of the first two smells don’t apply here. This is just bad technique.
- Long Parameter List: yuck. No. No. No.
This just sucks. I hate it. How can you tell what the hell a method is doing by its signature if the signature is 6 arguments? Maybe you could tell in Smalltalk, with named parameters, but we don’t use Smalltalk in Enterprise Land.
Even so, a long parameter list wreaks most of all because a lot of the time, some of the parameters don’t apply, and you have to pass garbage values. And that prompts the author of the method you’re using to litter the code with null checks. Before you know it, you’re doing too much in that method.
- Null Obsession / Null Considered Harmful: speaking of which, littering your code with null checks means you’re doing it wrong. I get so much push-back at work when I want to freaking initialize private members in a constructor.
Can you believe that? I don’t like checking for null. You shouldn’t have to check for the existence of an object in the first place. You have an object, and you tell it to do work. Null-checking is a technical detail and a billion dollar mistake.
- Type Switch in OO language: this one isn’t as bad as it sounds. You might have a legitimate reason not to subclass everything. Sometimes you can’t, like if the types are external to your system.
If the behavior associated with each type doesn’t extend beyond initialization of some attributes, then this is manageable inside a factory object of some sort. Beyond that, then you might as well subclass them, even if they are external.
- Data Class: do I even need to say it? The only possible exceptions are if you consciously define a layer of data structures and build an OO model on top of that, and export objects.
- Message Chain: I only tolerate these as exceptions to the Data Class smell. Otherwise, the coupling is tighter than the Year 2038 Problem.
- Inappropriate Intimacy / Feature Envy: herein lies the main reason why I don’t think domain objects should have just getters for everything, and not setters. If you’re a repository trying to persist one of those, you’ll suffer from Feature Envy. One solution is to create a Data Class and ask the domain object to fill it up.
Now you can argue that we merely moved the Feature Envy from the domain object to the Data Class. While technically correct, the repository is no longer treating the domain object like it is a domain object and a Data Class. The two concerns are separated.
I imagine you could take it a step further: let the repository define a Data Class, let the domain object fill it up, and then give the Data Class a method to do whatever it is that needs to be done, i.e. generate SQL:
equipmentExport := EquipmentSQLGenerator new.
db exec:(equipmentExport generateInsertString).
I am ready to accept your accolades
All of the lists for code smells seem biased towards OO programming. How come I never see code smell lists for procedural or functional programming?
On second thought, maybe I don’t want to know the answer to that.
|Announcer: You’re reading the EIP web-ring.|