Code smells are indicators that there might be something afoul in our code. Here is a list of some of the most important smells.

What are code smells?

A code smell is a characteristic of a piece of code that does not “feel right”. It is not necessarily a problem in itself and should be a hint at a possible problem. One swallow does not a summer make, and, equally, one single smell does not mean we have written bad code.

We might have had to work around some limitations, improve performance in a bottleneck, or there are other reasons that the code is not the perfect beauty we would like it to be. Usually though, if we can identify a lot of code smells in our code, that is strong evidence that we might be violating programming principles.

In this post, I want to compile a list of the most common and, for C++, most important code smells. There are lots more that I will not touch, e.g. testing smells. There also are a lot more smells related to object orientation than I will be able to include here, but since C++ is not a purely object oriented language, they have less merit than e.g. in Java.

With each smell, I will explain which programming principles might be violated and a standard way to get rid of it.

Long function

We have all seen them and probably produced our fair share ourselves: Functions that span dozens, maybe even hundreds of lines or more. Apart from the sheer amount of code to read and keep in mind in order to understand them, they tend to violate two of the major coding principles:

Single responsibility principle: long functions tend to do not one but several things at once, and in detail. That is also a violation of the single level of abstraction principle: Such a long function describes not only how to do things on a low level, but also in which order the things should be done, which is a higher level.

The solution is relatively simple: factor out the low-level details into their own functions. This leaves the original function at a high level, describing what has to be done. The low-level functions implement how it’s done. Repeat until the functions are short enough. You may find good names for the low-level functions in the comments that often group the long function content.

The number of lines that make a function too long is subjective. Purists say a function should not contain more than five or even three lines, which I deem very extreme. Others use “one screen height” as the unit of choice. Given that I know people who use their 27-inch screens in portrait mode that can be a bit much. I guess a soft limit around 20 lines is good.

However, that does not mean that a function of 15 lines is always OK – such a function could still violate SLoA and the SRP.

Primitive Obsession

The primitive obsession code smell appears to be fairly common. It means that we overuse the basic types of the language, but also of the standard library. The problem behind this smell is twofold.

The first is that our types lack proper naming and type safety. Imagine a function that takes three integers as parameters and returns another int. You will know the semantics of these parameters if they have good names, but for the returned int it can be a bit trickier. You’ll also have the problem of accidentally providing arguments in the wrong order.

Another example is the primitive obsession with standard library types. It’s easy to plug those together, sometimes too easy. What would you say a std::tuple<int, std::string, std::vector<std::vector<std::string>>> is for? Yeah, I don’t know either, but I’ve seen monstrosities like that. Using a properly named handwritten type can go a long way here.

The second problem with primitives is that we more often than not have logic associated to our variables that go beyond the built-in properties of the basic types. That std::string variable you called title probably has some constraints, like minimum and maximum length, no special characters and so on. That logic often is scattered in every class that has or deals with a title. Instead, we should use a strong type, putting the logic into its own Title class.

Read more about the topic in Jonathan Boccara’s “strong types” blog series.

Data Clump

The data clump code smell is a special form of the primitive obsession. Often we see variables travel in loose packs. The standard example is having two floats x and y as parameters for a host of functions and as member variables, instead of explicitly using a Point struct or class.

The problems are similar to those of the primitive obsession, as is the solution. And no, a std::pair<float, float> does not quite cut it.

Complicated boolean expressions

Who hasn’t seen incarnations of this code smell: if( followed by a dozen lines full of comparisons and boolean logic, lost in a forest of parentheses to maybe get the precedence right. And everyone who has to read it is asking the same question: “What the heck are we actually testing for?”

The solution is simple: Add some meaningful names. Assign some of the sub-expressions to variables with meaningful names, breaking up the complex expression into several less complex ones. And, since the whole expression typically is only an implementation detail, factor it out into its own function.

Deeply nested control flow

Another classic code smell: Two layers of if statements inside a loop under a case label, and the whole switch block neatly wrapped into a try/catch. Not a problem these days, because our ultra wide screens can handle those lines starting at column 170 without having to scroll, right?

The problem is the amount of complexity we have to keep in mind while reading functions like that. The levels of abstractions of those different control flows are not likely to be the same, so SLoA is probably violated, and all that nesting usually comes results in a long function smell.

There are two techniques to handle this: the first is to simply factor out a function for any loop body, if/else body, case label and so on. It will not reduce the actual, total complexity of the code, but it will ease our parsing of the functionality by separating the levels of abstraction.

The other technique is to use early returns aka. guard clauses. It means that the code

if (everythingElseOk()) {
  doSomething;
}

can be translated to

if (somethingIsWrong()) {
  return;
}

doSomething();

It is the same logic, but somehow our minds are better in handling the second alternative. In the first case, we automatically try to keep in mind all the conditions our code is wrapped into. In the second case, those checks are in the past and we can concentrate on the actual logic.

Wrap up

There are a few more code smells that are frequent, but also easy to get rid of:

• Redundant comments hurt readability, just remove them.
• Dead code makes your code seem more complex than it is. Just throw it away.
• Magic numbers and strings don’t tell the reader what they mean. Use named constants instead.
• Duplicated code is a maintenance nightmare. Extract it into a function that you can call where needed.

Of course, there are a lot more code smells, but I think these are the most important ones. I will touch the others in the future.

Remember that code smells are only hints and the underlying problem may run much deeper. Rigorously cleaning up the smells may feel very good, but fixing the symptoms will not cure the disease.