Do code reviews find bugs?

I recently overheard this: “Code reviews are a waste of time – they don’t find bugs! We should rely on our tests and skip all this code review mambo-jumbo.”

And I agree – tests are important. But they won’t identify many issues code reviews will. Here are a few examples.

Unwanted dependencies

Developers often add dependencies they could easily do without. Bringing in libraries that are megabytes in size only to use one small function or relying on packages like true may not be worth the cost. Code reviews are a great opportunity to spot new dependencies and discuss the value they bring.

Potential performance issues

In my experience, most automated tests use only basic test inputs. For instance, tests for code that operates on arrays rarely use arrays with more than a few items. These inputs might be sufficient to test the basic functionality but won’t put the code under stress.

Code reviews allow spotting suboptimal algorithms whose execution time rapidly grows with the input size or scenarios prone to combinatorial explosion.

The latter bit our team not too long ago. When reviewing a code change, one of the reviewers mentioned the possibility of a combinatorial explosion. After discussing it with the author, they concluded it would never happen. Fast forward a few weeks, and our service occasionally uses 100% CPU before it crashes due to running out of memory. Guess what? The hypothetical scenario did happen. Had we analyzed the concern mentioned in the code review more thoroughly, we would’ve avoided the problem completely.

Code complexity and readability

Computers execute all code with the same ease. They don’t care what it looks like. Humans are different. The more complex the code, the harder it is to understand and correctly modify. Code review is the best time to identify code that will become a maintenance nightmare due to its complexity and poor readability.

Missing test coverage

The purpose of automated tests is to flag bugs and regressions. But how do we ensure that these tests exist in the first place? Through a code review! If test coverage for a proposed change is insufficient, it is usually enough to ask in a code review for improving it.

Bugs? Yes, sometimes.

Code reviews do find bugs. They don’t find all of them, but any bug caught before it reaches the repository is a win. Code reviews are one of the first stages in the software development process making them the earliest chance to catch bugs. And the sooner a bug is found, the cheaper it is to fix.

Conclusion

Tests are not a replacement for code reviews. However, code reviews are also not a replacement for tests. These are two different tools. Even though there might be some overlap, they have different purposes. Having both helps maintain high-quality code.

Storytime

One memorable issue I found through a code review was the questionable use of regular expressions. My experience taught me to be careful with regular expressions because even innocent-looking ones can lead to serious performance issues. But when I saw the proposed change, I was speechless: the code generated regular expressions on the fly in a loop and executed them.

At that time, I was nineteen years into my Software Engineering career (including 11 years at Microsoft), and I hadn’t seen a problem whose solution would require generating regular expressions on the fly. I didn’t even completely understand what the code did, but I was convinced this code should never be checked in (despite passing tests). After digging deeper into the problem, we found that a single for loop with two indices could solve it. If not for the code review, this change would’ve made it to millions of phones, including, perhaps, even yours, because this app has hundreds of millions of downloads across iOS and Android.

The self-inflicted pain of premature abstractions

Premature abstraction occurs when developers try making their code very general without a clear need. Examples of premature abstraction include:

  • Creating a base class (or interface) even though there is only one known specialization/implementation
  • Implementing a more general solution and using it for one purpose, e.g., coding the visitor pattern, only to check if a value exists in a binary search tree
  • building a bunch of microservices for an MVP (Minimum Viable Product) application serving a handful of requests per minute

I have seen many mid-level and even senior software developers, myself included, fall into this trap. The goal is always noble: to come up with a clean, beautiful, and reusable architecture. The result? An unnecessarily complex mess that even the author cannot comprehend and which slows down the entire team.

Why is premature abstraction problematic?

Adding abstractions before they are needed adds needless friction because they make the code more difficult to read and understand. This, in turn, increases the time to review code changes and risks introducing bugs just because the code was misunderstood. Implementing new features takes longer. Performance may degrade, thorough testing is hard to achieve, and maintenance becomes a burden.

Abstractions created when only one use case exists are almost always biased toward this use case. Adding a second use case to fit this abstraction is often only possible with serious modifications. As the changes can’t break the first use case, the new “abstraction” becomes an awkward mix of both use cases that don’t abstract anything.

With each commit, the abstraction becomes more rooted in the product. After a while, it can’t be removed without significantly rewriting the code, so it stays there forever and slows the team down.

I witnessed all these problems firsthand when, a few years ago, I joined a team that owned an important functionality in a popular mobile app. At that time, the team was migrating their code to React Native. One of the foundations for this migration was a workflow framework implemented by a couple of team members that was inspired by Operations from Apple’s Foundation Framework. When I joined the team, the workflow framework was a few weeks late but “almost ready.” It took another couple of months before it was possible to start using it to implement simple features. Only then did we find out how difficult it was! Even a simple functionality like sending an HTTP request required writing hundreds of lines of code. Simple features took weeks to finish, especially since no one was willing to invest their time reviewing huge diffs.

One of the framework’s features was “triggers,” which could invoke an operation automatically if certain conditions were satisfied. These triggers were a source of constant performance issues as they would often unexpectedly invoke random operations, including expensive ones like querying the database. Many team members struggled to wrap their heads around this framework and questioned why we needed it. Writing simple code would have been much easier, faster, and more enjoyable. After months of grinding, many missed deadlines, and tons of functional and performance issues, something had to be done. Unfortunately, it turned out that removing the framework was not an option. Not only did “the team invest so much time and effort in it,” but we also released a few features that would have to be rewritten. Eventually, we ended up reducing the framework’s usage to the absolute minimum for any new work.

What to do instead?

It is impossible to foresee the future, and adding code because it might be needed later rarely ends well. Rather, writing simple code, following the SOLID principles, and having good test coverage are encouraged. This way, you can add new abstractions later when you do need them without introducing regressions and breaking your app.

std::optional? Proceed with caution!

The std::optional type is a great addition to the standard C++ library in C++ 17. It allows to end the practice of using some special (sentinel) value, e.g., -1, to indicate that an operation didn’t produce a meaningful result. There is one caveat though – std::optional<bool> may in some situation behave counterintuitively and can lead to subtle bugs.

Let’s take a look at the following code:

  bool isMorning = false;
  if (isMorning) {
    std::cout << "Good Morning!" << std::endl;
  } else {
    std::cout << "Good Afternoon" << std::endl;
  }

Running this code prints:

Good Afternoon!

This is not a surprise. But let’s see what happens if we change the bool type to std::optional<bool> like this:

  std::optional<bool> isMorning = false;
  if (isMorning) {
    std::cout << "Good Morning!" << std::endl;
  } else {
    std::cout << "Good Afternoon!" << std::endl;
  }

This time the output is:

Good Morning!

Whoa? Why? What’s going on here?

While this is likely not what we expected, it’s not a bug. The std::optional type defines an explicit conversion to bool that returns true if the object contains a value and false if it doesn’t (exactly the same as the has_value() method). In some contexts – most notably the if, while, for expressions, logical operators, the conditional (ternary) operator (a complete list can be found in the Contextual conversions section on cppreference) – C++ is allowed to use it to perform an implicit cast. In our case it led to a behavior that at the first sight seems incorrect. Thinking about this a bit more, the seemingly intuitive behavior should not be expected. An std::optional<bool> variable can have one of three possible values:

  • true
  • false
  • std::nullopt (i.e., not set)

and there is no interpretation under which the behavior of expressions like if (std::nullopt) is universally meaningful. Having said that, I have seen multiple engineers (myself included) fall into this trap.

The problem is that spotting the bug can hard as there are no compiler warnings or any other indications of the issue. This is especially problematic when changing an existing variable from bool to std::optional<bool> in large codebases because it is easy to miss some usages and introduce regressions.

The problem can also sneak easily to your tests. Here is an example of a test that happily passes but only due to a bug:

TEST(stdOptionalBoolTest, IncorrectTest) {
  ASSERT_TRUE(std::optional<bool>{false});
}

How to deal with std::optional<bool>?

Before we discuss the ways to handle std::optional<bool> type in code, it could be useful to mention a few strategies that can prevent bugs caused by std::optional<bool>:

  • raise awareness of the unintuitive behavior of std::optional<bool> in some contexts
  • when a new std::optional<bool> variable or function is introduced make sure all places where it is used are reviewed and amended if needed
  • have a good test unit coverage that can detect bugs caused by introducing std::optional<bool> to your codebase
  • if feasible, create a lint rule that flags suspicious usages of std::optional<bool>

In terms of code there are few ways to handle the std::optional<bool> type:

Compare the optional value explicitly using the == operator

If your scenario allows treating std::nullopt as true or false you can use the == operator like this:

std::optional<bool> isMorning = std::nullopt;
if (isMorning == false) {
  std::cout << "It's not morning anymore..." << std::endl;
} else {
  std::cout << "Good Morning!" << std::endl;
}

This works because the std::nullopt value is never equal to an initialized variable of the corresponding optional type. One big disadvantage of this approach is that someone will inevitably want to simplify the expression by removing the unnecessary == false and, as a result, introducing a regression.

Unwrap the optional value with the value() method

If you know that the value in the given codepath is always set you can unwrap the value by calling the value() method like in the example below:

    std::optional<bool> isMorning = false;
    if (isMorning.value()) {
      std::cout << "Good Morning!" << std::endl;
    } else {
      std::cout << "Good Afternoon!" << std::endl;
    }

Note that it won’t work if the value might not be set – invoking the .value() method if the value was not set will throw the std::bad_optional_access exception

Dereference the optional value with the * operator

This is very similar to the previous option. If you know that the value on the given code path is always set you can use the * operator to dereference it like this:

std::optional<bool> isMorning = false;
if (*isMorning) {
  std::cout << "Good Morning!" << std::endl;
} else {
  std::cout << "Good Afternoon!" << std::endl;
}

One big difference from using the value() method is that the behavior is undefined if you dereference an optional whose value was not set. Personally, I never go with this solution.

Use value_or() to provide the default value for cases when the value is not set

std::optional has the value_or() method that allows providing the default value that will be returned if the value is not set. Here is an example:

std::optional<bool> isMorning = std::nullopt;
if (!isMorning.value_or(false)) {
  std::cout << "It's not morning anymore..." << std::endl;
} else {
  std::cout << "Good Morning!" << std::endl;
}

If your scenario allows treating std::nullopt as true or false using value_or() could be a good choice.

Handle std::nullopt explicitly

There must have been a specific reason you decided to use std::optional<bool> – you wanted to enable the scenario where the value is not set. Now you need to handle this case. Here is how:

    std::optional<bool> isMorning = std::nullopt;    
    if (isMorning.has_value()) {
      if (isMorning.value()) {
        std::cout << "Good Morning!" << std::endl;
      } else {
        std::cout << "Good Afternoon!" << std::endl;
      }
    } else {
      std::cout << "I am lost in time..." << std::endl;
    }

Fixing tests

If your tests use ASSERT_TRUE or ASSERT_FALSE assertions with std::optional<bool> variables they might be passing even if they shouldn’t as they suffer from the very same issue as your code. As an example, the following assertion will happily pass:

ASSERT_TRUE(std::optional{false});

You can fix this by using ASSERT_EQ to explicitly compare with the expected value or by using some of the techniques discussed above. Here are a couple of examples:

ASSERT_EQ(std::optional{false}, true);
ASSERT_TRUE(std::optional{false}.value());

Other std::optional type parameters

We spent a lot of time discussing the std::optional<bool> case. How about other types? Do they suffer from the same issue? The std::optional type is a template so its behavior is the same for any type parameter. Here is an example with std::optional<int>:

    std::optional<int> n = 0;
    if (n) {
      std::cout << "n is not 0" << std::endl;
    }

which generates the following output:

n is not 0

The problem with std::optional<bool> is just more pronounced due to the typical usages of bool. For non-bool types it is fortunately no longer a common practice to rely on the implicit cast to bool. These days it is much common to write the condition above explicitly as: if (n != 0) which will compare with the value of as long as it is populated.

Enums and Exhaustive switch statements in C++

Exhaustive switch statements are switch statements that do not have the default case because all possible values of the type in question have been covered by one of the switch cases. Exhaustive switch statements are a perfect match when working with scoped enum types. This can be illustrated by the following code:

#include <iostream>
#include <string>
enum class Color {
Red,
Green,
Blue,
};
std::string getColorName(Color c) {
switch(c) {
case Color::Red: return "red";
case Color::Green: return "green";
case Color::Blue: return "blue";
}
}
int main() {
std::cout << "Color: " << getColorName(Color::Green) << std::endl;
return 0;
}
view raw example.cpp hosted with ❤ by GitHub

Now, if a new enum member is added:

enum class Color {
Red,
Green,
Blue,
Yellow,
};

the compiler will show warnings pointing to all the places that need to be fixed:

enum.cpp:12:12: warning: enumeration value 'Yellow' not handled in switch [-Wswitch]
switch(c) {
^
enum.cpp:12:12: note: add missing switch cases
switch(c) {
^
enum.cpp:17:1: warning: non-void function does not return a value in all control paths [-Wreturn-type]

If you configure warnings as errors, which you should do if you can, you will have to address all these errors to successfully compile your program.

Without the exhaustive switch, the compiler would happily compile the program after adding the new enum member because it would be handled by the default case. Even if the default case was coded to throw an exception to help detect unhandled enum members, this exception would only be thrown at runtime which could be too late to prevent failures. In a bigger system or application there could be many switch statements like this and without the help from the compiler it can be hard to find and test all of them. To sum up – exhaustive switch statements help quickly find usages that need to be looked at and fixed before the code can ship.

So far so good, but there is a problem – C++ allows this:

Color color = static_cast<Color>(42);
view raw cast.cpp hosted with ❤ by GitHub

Believe it or not, this is valid C++ as long as the value being cast is within the range of the underlying enum type. If you flow an enum value created this way through the exhaustive switch it won’t match any of the switch cases and since there is no default case the behavior is undefined.

The right thing to do is to always use enums instead of mixing integers and enums which ultimately is the reason to cast. Unfortunately, this isn’t always possible. If you receive values from users or external systems, they would typically be integer numbers that you may need to convert to an enum in your system or library and the way to do this in C++ is static casting.

Because you can never trust values received from external systems, you need to convert them in a safe way. This is where the exhaustive switch statement can be extremely useful as it allows to write a conversion function like this:

Color convertToColor(int c) {
auto color = static_cast<Color>(c);
switch(color) {
case Color::Red:
case Color::Green:
case Color::Blue:
return color;
}
throw std::runtime_error("Invalid color");
}
view raw convert.cpp hosted with ❤ by GitHub

If a new enum member is added, the compiler will fail to compile the convertToColor function (or at least will show warnings), so you know you need to update it. For enum values outside of the defined set of members the convertToColor throws an exception. If you use a safe conversion like this immediately after receiving the value, you will prevent unexpected values from entering your system. You will also have a single place where you can detect invalid values and reject and log incorrect invocations.