Those of us who develop software are familiar with all sorts of bonehead mistakes. I use “bonehead” in this context as a term of endearment — we all do stupid things like forget to initialize some value or forget to close a brace now and then. And there are plenty of discussions, in books and elsewhere, about the sorts of mistakes that new programmers make.
That’s not what I’m going to talk about today.
What I want to talk about today are the most common mistakes I see experienced software developers make on a somewhat regular basis. Not every developer makes all these mistakes, of course, but if you work for a big enough company you’ll see all of these at different times. They bug me. I’ve committed some of the sins in here myself, and I bug myself when I realize that. Note that I’m deliberately keeping this list fairly low level, and not addressing larger questions of design. The overall problems I’m aiming for are the sorts of things when you’re reading someone else’s code (or your own) and have that “What? Didn’t you go to school and have 10 years of experience that should have told you not to do something lame like this?” moment.
I’ve heard things like this referred to as “antipatterns,” but I think that’s an unnecessarily highfalutin’ name for what can more accurately and concisely be described as: screwups. So here we go.
1. Improperly maintained namespace.
Software projects tend to grow over time, like the blob. Assumptions about your environment that you made early on in the project don’t always turn out to be true. Do yourself a favor; pick a prefix for every global variable and every extern or public function or class, and use it. Maintain a naming scheme so that if your project has many files — and it always does — I can tell just from looking at an identifier where to find it.
2. Misuse of asserts.
I see this all the time, and it drives me up a wall:
ASSERT(status == SUCCESS);
Assertions aren’t there for you to claim that errors are impossible; asserts are about finding impossible conditions — programming errors. “Out of disk space” is not a programming error: asserting on it is never right (although, depending on the nature of what you’re doing, panicking might be correct). Contrariwise, something like decrementing the refcount on some resource and finding that it is less than 0 is a programming error, and is a good candidate for an assert.
If a function really, honestly, cannot ever return any value other than SUCCESS, then it should return void.
There’s a school of thought that says that asserts should never be used, particularly in any code that is going to an end-user. I don’t know what sort of crack these people are smoki I’m not going to get involved in that particular argument, but if you do use asserts, you should use them correctly.
3. Ignoring return codes.
This is even more annoying than the last one. If it’s valid for you to ignore a return code from a call — any call — then at a bare minimum I want a detailed comment explaining why it’s OK.
4. Making everything extern/public.
If a function or method is intended to only be used internally, don’t rely on me, the user of your function, to be polite and not use it. Experience has shown that users — including me — are stupid, and if we can use something we’re not supposed to, we will. You can smack big banners on your functions that say “Don’t use this, you dope”, and we will still use them. Think of this as self-defense. If you make a function or method static or private, then we can’t use it without checking in a change to your code, and then when it bites us, you can point to our dumb checkin where we changed your nice, safe code, and it will be our fault, not yours. Don’t make internal functions extern. Keep them secret. Keep them safe.
If you write the API for a given part of your code first, then it’s really easy: the API calls are extern. Everything else is static. You’d be surprised how many people who should know better just make everything extern by default.
5. Not thinking about versioning until “later”.
The voice of experience: if you don’t figure out how to version interfaces right from the get go, it will be a nightmarish whirling calliope of hatred, pain, and misery later on.
6. Making a change to a changelist after running the regression tests.
Look. I understand. I’ve been there. I know all you did was update a comment. I know you just made some change that couldn’t possibly, in a million years, cause anything to fail. I know the regression test suite takes a long time to run. That change you made was totally trivial, and couldn’t possibly break anything. I feel your pain. But the fact is, those are inevitably the checkins that break the build. No one knows why. It just works that way. So run the tests again before pulling that trigger.
7. Sending a human to do a robot’s job.
Any piece of software that is more than trivial is too big for a human to debug properly without help. I’ll tell you my dirty little secret: I hate code reviews. Yes, they’re valuable in their own special way, and yes, I’ve caught bugs in other people’s code in a review (and others have caught bugs in my code), but in terms of pure effort/reward ratio, I have gotten more out of letting the robots debug my code than out of the hours I’ve spent in code reviews. Use Purify to find memory leaks. Use Coverity to find other types of programmatic mistakes. Run gcov to find out what parts of your code your tests aren’t exercising. If speed is a problem, profile your code to find out where it’s slow, rather than looking at the code and guessing.
8. Not obeying the robots
Really, a special case of the last item. Turn on all of your compiler’s warnings. Fix your code so it doesn’t trip any of them. If the language you’re using has a lint program, run it. Listen to what it says. Fix your code so lint is happy. To quote Peter van der Linden, from his excellent book Expert C Programming:
Lint is your software conscience. It tells you when you are doing bad things. Always use lint. Listen to your conscience.Separating lint out from the compiler as an independent program was a big mistake that people are only now coming to terms with. It’s true that it made the compiler smaller and more focused, but it was at the grievous cost of allowing bugs and dubious code idioms to lurk unnoticed. Many, perhaps most, programmers do not use lint by default after each and every compilation. It’s a poor trade-off to have buggy code compiled fast.
Words to live by.
That’s my list. These are things that I see people mess up with some regularity. What would you add to this list?
Additional Resources
- If you’re a C programmer, you need a copy of Expert C Programming: Deep C Secrets.
- Here’s a nice primer on what asserts are, and how to properly use them. Sun’s guide to asserts in Java is pretty good, too.
Premature reuse:
Designing component X to be resuable by many applications without first obtaining the experience of actually deploying component X in one context is the kiss of death and dismemberment.
This is related to premature optimization. The problem is that without concrete experience with a component and its requirements its impossible to abstract one level up and generalize the requirements and the implementation. The result is that you end up with a “resuable” component that is only good for one set of requirements, the ones that you thought of first.
Consider how many times “container” classes have been reimplemented. You’d think we’d get that right by now, but we haven’t.
“Turn on all your compiler warnings and fix the problems” is a recipe for disaster when you have kernel code that builds on like 10 platforms, typically multiple OS versions for each, going back to like 1995. If you’re bored, try it sometime.
My experience from games was that building the same code on multiple compilers, turning on all the compiler warnings and fixing the problems, generally worked pretty well for cleaning up a codebase.
At least when we could fix the bugs…
I don’t think it’s a recipe for disaster. It might be a recipe for that particular build failing, but obviously if you have some limitation that makes some particular guideline I give inapplicable — such as “the people who wrote this kernel were idiots, so their header files puke warnings when you compile with all warnings turned on,” then you modify your behavior accordingly.
Your point is a really good example, by the way, of how lousy code begets more lousy code. Somewhere there’s a guy writing a kernel device driver who wants to write good code, but because some nameless kernel *cough*LINUX*cough* is dripping with garbage and won’t build with -Werror -Wall, he basically _can’t_ use the robots to help him write good code.
I feel sorry for that guy. I bet he wishes that the people that wrote those kernels had done their jobs properly. If they had, he’d be able to use better tools to help him write better code.
3a. Functions that return void
This is my pet peeve. Declaring a function to return void translates to “for all eternity I hereby declare this function will always succeed.” It is rare when you can actually make this claim and even rarer that it remains true over time. Save us all the effort, declare your function to return a value – we’ll add the error handing up front and then in six months when you update the function we’ll both be glad you chose the higher calling.
7.a. if you are using code reviews, understand the goals, and make sure you have the right kind of review for the goal. are you using reviews to make sure idiots don’t check in dumb code (code reviewed by a small senior team), reviews to make sure you are writing good code (stand up in front of everyone and do a walkthrough), or reviews to make sure new code makes sense with old code (approval of last few people to touch that codebase and/or the architects of said codebase).
7.b. make sure your tools cooperate with and/or enforce your development process rather than fighting it.
8.a. if you have access to more than one compiler, use both for testing. different compilers catch different code issues.
9. in comments, document intent as well as behavior. this makes it easier for people who do read comments to understand how to write to the interface so that all hell doesn’t break loose if you change the behavior.
(Insert religious zealotry about the superiority of BoundsChecker over Purify. YMMV).
Yes, about the profiling for speed. I would say this even stronger than you did, that in my experience most assumptions about what code is causing slowness are just plain wrong. Use a profiler, or put in your own instrumentation and get the facts.
C++ specific – make your destructors virtual by default. You may not remember to change the signature when you subclass three years later.
>3a. Functions that return void
>This is my pet peeve. Declaring a function to
>return void translates to “for all eternity I
>hereby declare this function will always
>succeed.”
This isn’t true for languages that provide exceptions. In fact, if your language provides exceptions, you should use them to signal error conditions rather than relying on the caller to check a status code, becuase they force the caller to deal with the error in some manner rather than just ignoring it.
I’d believe the comment regarding expections if in my past I hadn’t seen things like this before:
try {
/* my entire program that ignores exceptions */
} catch {
print(“something bad happened”);
}
Software Development Considered Harmful
Software Development Considered Harmful
>I’d believe the comment regarding expections if in >my past I hadn’t seen things like this before:
>
>try {
>/* my entire program that ignores exceptions */
>} catch {
>print(“something bad happened”);
>}
just because you give a man a shovel, doesn’t mean he is going to use it to dig, many shovels make good leaning posts for the lazy.