A couple coding pet peaves

I’ve already written about one coding pet peave but I felt that I should share a few more. …And no, this is not an April Fool’s Day joke.

The pet peaves in this post are about needless use of constants, the bad name that early returns have earned, and copy-and-pasting code.

Don’t use a constant just because you were taught that you should never have any literals in your code. Here’s an example of being stupid:

const char* SpdUtils::debugName = "XML_SpdUtil";
ModuleDbgApi SpdUtils::dbg(debugName, false);

This is only stupid if SpdUtils::debugName is used only once. In the actual code that this was taken from this was the only occurance where that symbol was used. In this case the constant buys absolutely nothing except perhaps to make the coder feel better about themselves. The real pet peave here is people that follow guidelines with out thinking. There is a reason that you should use constants most of the time but that doesn’t mean you should use them every time. This is also true of the goto. Contrary to what you’ve probably been taught goto statements are not always evil. Improper use of them is evil but there are good scenarios where a goto is justified.

Another pet peave of mine is when people put implementation code in header files. It’s very tempting to do but you shouldn’t do it. You shouldn’t even do this for so-called “getters” and “setters.” Implementation belongs in implementation files. Declarations belong in headers files. If you put implementation code in header files then you’ve just forced someone reading your code to have to grep your header files and your implementation files when they are reading your code. That adds a lot of noise that makes the readers job more difficult.

When did people start believing that an early return from a function is evil? I’ve seen an awful lot of code where people jump through hoops avoiding an early return. They invariably end up with a function 5 levels deep with only one valid execution path. To get to the meat of the function the reader has to delve many levels into the code. In many cases an early return makes the function far more readable. A function typically has a single goal. Anything that distracts from that goal is something that the reader shouldn’t have to focus too much on. A good practice is to do your checking up front. Immediately return when you’ve detected some error state that prohibits the function from completing its task. The flow of the code then points the reader through the real logic of the function and not the error handling logic.

Many people will argue that early returns promote memory leaks and other subtle bugs. This is true if you have a poorly written function. If your function is sufficiently short it should not be a difficult task for a reader to easily see what sort of cleanup is required. However, there are always exceptions. Sometimes early returns are not the right choice. Think about the particular situation you are faced with and choose the most readable solution.

Finally, do not copy and paste code. This ought to be obvious to people but I’ve seen enough code like this to know that it’s not obvious to them. If you have code that is similar enough that you might want to copy and paste it somewhere else then you ought to write a function. It’s a virtual certainty that you are going to have to change the code in question in the future. By copying it you now have to edit it in two places. What will happen is that you will forget about the multiple copies of similar code and only change one leaving bugs.

This entry was posted in Code. Bookmark the permalink.

Leave a Reply

Fill in your details below or click an icon to log in:

WordPress.com Logo

You are commenting using your WordPress.com account. Log Out /  Change )

Google+ photo

You are commenting using your Google+ account. Log Out /  Change )

Twitter picture

You are commenting using your Twitter account. Log Out /  Change )

Facebook photo

You are commenting using your Facebook account. Log Out /  Change )

Connecting to %s