Common code smells by Ólafur Waage

This post was triggered by a Twitter thread by Ólafur Waage about the common mistakes he observed during code reviews over a period of four months. Some of these were not news to me, but I had never seen them collected as a set in a single place.

I have added and/or rephrased some passages based on personal experience.

One function that does everything

Many people think they should only create functions if they are reused. No. Create functions for anything that can be thought of as a generic action, or to document the steps of a process. This greatly helps readability and structure.

Early exits

int foo() 
{ 
  if (x) 
  { 
    // lots of code ... 
     return y; 
  }
  return 0; 
}

Reverse that statement. As it stands, the reader has to go through a lot to make sure the “bad” exit case is OK. That takes up mental space and makes reasoning about your code more difficult. If it’s reversed, nothing below the if matters for the x == false case.

int foo() 
{ 
  if (!x) return 0;
  // lots of code ... 
  return y; 
}

This also reduces the number of braces and indentation levels.

Avoid magic numbers

if (something > 5) ...

What is this 5? Give it a name and put it somewhere; this number has context, so show it. Put it in the scope that makes sense. It does not have to be in a big scope or be reused; inside the function itself is fine too.

const int maximumTemperature = 5;
if (something > maximumTemperature ) ...

Notice that your brain is now probably trying to make sense of the function, and questions arise like:

• should this be a floating-point number?
• are these in Celsius or Fahrenheit?

These are good questions, and sometimes the name of the variable can answer them. This also raises the question: shouldn’t that be part of the type of the variable, so that assigning degrees Celsius to degrees Fahrenheit would be a compiler error? This can be accomplished with the Boost.Units library. There is an excellent talk on this subject by Robert Ramey from CppCon 2015.

One class for everything

This is similar to one function for everything. If you can categorize the actions within a class, you can make classes out of those categories. It is quite a burden to maintain 6000-line classes. For example, if you have a 10-step process, create a class, give it the name of the process, and create 10 functions to express the steps.

Inconsistent formatting style

Reading code is mentally straining: not only do you have to parse what each line is doing, you also need to parse the structure. If the style is inconsistent, that becomes harder. Which style doesn’t matter, just make it the same everywhere. Tools can be a big help here. An example is clang-format, which can format your code, or just a selection of it, to conform to a style agreed on by the team.

Comments everywhere

// Increment value by one 
i++;

My issue with comments is that you now have two meanings for everything: the line has a meaning, and so does the comment. They must match, or else there is trouble. Add a comment when something needs explaining, when the code by itself would not make sense. But you can almost always just make a function that describes the action, and then no comment is needed. The compiler does not read comments (obviously), so if a comment describes a precondition that must hold, just check the precondition, or use static_assert if possible.

Performing complex actions in if statements

if (myFoo->Action(aBar + aBaz->GetStuff(18)))

The only time you should put direct actions in if statements is when you want to block execution with an && (so-called short-circuit evaluation). Otherwise, put the result into a variable and then ask the question.

Stringly typed

if (a == "foo") {...} else if(a == "bar") {...}

Unless you are actually parsing string data, you should use classes, enums, or at least numbers to determine what is what. Strings are slower to evaluate and offer no type safety.