C++ is a powerful language — but it’s also a highly complex one, and that complexity makes it highly vulnerable to misinterpretation and overcomplication. It’s easy to miss bugs in C++ that would stand out like a sore thumb in simpler languages — and when the bugs do appear in production it might be harder to locate them compared to other languages.
In short, C++ needs to be handled with care — and reviewed with an eagle eye.
This blog is divided into two parts. In part one, we’ll discuss more general aspects of code review. In part two, we’ll dive into specific C++ code review topics and build a checklist for C++ code reviews.
Why is C++ code review so important?
No matter what language you’re coding in, it’s always difficult to see the bugs and errors in your own code, either because you wrote it to begin with, or because you’re simply unaware that what you’re doing might be wrong. Having someone review our code is a great way around this issue.
In addition, the mere discussion of code between two people or more raises new questions and helps in pinpointing the important issues.
The above is true for any programming language you use, but the complexity of C++ makes it even more critical, requiring at least one extra set of eyes to review your code, is essential.
What comes before code review?
There are two things that you need before submitting your code for review.
The first is passing a static code analysis and being able to explain any warnings you find that could be false positives or shouldn’t be handled for any reason. There are great static code analysis tools out there – both open source and commercial – and there is no reason not to use them. We assume of course that you do not leave any compiler warnings, but in the rare cases where you have a really good reason to ignore a compiler warning, it should of course also have good explanation prepared ahead (e.g. as a comment above a pragma declaration, instructing the compiler to ignore the warning).
The second thing you need is to actually test your code. You’ll want to run unit tests that generate enough coverage of your code, preferably including errors and edge cases. Without testing there is no sense in conducting a code review, as you can’t tell whether your code actually works. Moreover, one of the items to be reviewed in a code review are the tests. If you add tests after code review, make sure that changes made in the code, following test failures, do not escape code review.
Both static code analysis and good unit tests can reduce issues from propagating into the code review and let you fix things beforehand, but they can’t catch every single issue.
For instance, if you got the requirements wrong from the start, neither great tests nor static analysis will reveal the issues there. On the other hand, code review considers the requirements and helps you map them to the code and tested scenarios. This lets you find inconsistencies between the requirements and the actual code.
In a recent survey by SmartBear, 24% of respondents pointed at code review as the number-one way a company can improve the quality of its code. With unit testing being second runner and static code analysis dragging behind. It’s not that unit testing and static code analysis are not important, it might be that respondents were seeing code review as something that tend to be sometimes neglected, or not well performed. But it is clear that unit tests and static code analysis do not replace the need for code review
The main things to look for in code review
The ultimate goal of code review is to locate points where code is faulty or can be improved. This includes looking for:
- Whether the code meets the requirements
- Whether code fits coding conventions and guidelines
- Logical errors, bug-prone code, potential bugs and actual bugs including concurrency issues
- Design flaws, inefficiencies, and future readiness
- Places where code can be simplified
- Whether we have good unit tests that would also pick up future bugs
Before elaborating on the above list, let’s look at the technicalities of code review and the ways to perform code review.
Types of code review
There are several ways to conduct code review:
The classical code review meeting
The major stakeholders meet with the developer(s) who wrote the code in question. The size of the group depends on the importance and complexity of the reviewed code. The review may include developers from other teams, system engineers, product management and testing engineers. Some see it as a waste of time to include non-developers in a code review, but if the session is focused on the way complicated requirements were implemented, they might have valuable insights. It may also simply include the developer and a single reviewer.
Issues raised during the code review can be fixed immediately if they’re “quick wins” (e.g., renaming variables and functions or adding short comments). Other issues should be documented to be handled later. At the end of the code review, there should be a decision made as to whether the code can be integrated (after fixing minor comments if any) or should be presented again for another review.
It is to be noted that the meeting described can be physical or virtual.
It’s common practice to have someone on the team who didn’t write the code present it, with the coder only being called on if something is unclear. This helps ensure that the code is generally readable. On the other hand, it can be more efficient to have the coder themselves present their code. It’s worth trying both ways to see which fits your team better.
Pros:
- A discussion is a great tool to raise up questions that wouldn’t be raised otherwise.
- The code implementer can respond to comments, resolving some of them on the spot, and creating a fruitful discussion for others.
Cons:
- Hard to go into questions that require in-depth investigation (which would probably go off-line and wouldn’t be closed in the same meeting).
- Time-consuming and less efficient, especially if performed in a large group – not every discussion is relevant for all participants.
- Need to find a suitable timeslot that fits all stakeholders.
Offline code review
A developer completes their changes to the code and asks for a code review via the source control, as a pull request, or via other code review tool. Issues raised by the reviewers are opened as issues in the source control or in the code review tool. The developer can fix the issues or comment on them. The code review cycle ends when all fixes and comments are approved.
Pros:
- Great for remote workers or teams spread out over a wider geographic area and in different time zones.
- Reviewers can perform their reviews at their own convenience.
- Each reviewer can focus on different aspects of the code.
Cons:
- No discussion and feedback between the team members.
- Questions are opened as issues, even if the developer had a simple answer that would eliminate the need for opening this issue. Then, if the developer just closes the issue (because there is a good answer for this issue) we still don’t know if the one opening the issue is okay with the answer.
- It might be that the review did not cover the code sufficiently.
Over-the-shoulder
Once a developer wrote the code, another team member reviews it with the original developer watching — you guessed it — over their shoulder, walking through any errors and making suggestions. In this approach, the code review is much less formal.
Pros:
- Great for smaller teams where this sort of thing can be done informally and in person.
- Performed as close as possible to the finalization of the code when the code is still fresh in the writer’s mind and being easy to make fixes and adjustments.
Cons:
- There might be items or parts of the code that would be neglected and not reviewed.
- In many cases, such a review would be performed before conducting unit tests, thus not reviewing the final version of the code.
Does pair-programming eliminate the need in code review?
In pair programming, two developers work on the same machine. One person codes while the other provides advice and feedback as they work, occasionally switching roles. This approach is part of the Extreme Programming (XP) methodology. It’s great for developers in the early stages of their career, but not very efficient for all development tasks. Moreover, it’s useful when working on a complex problem that could benefit from two heads, but often this can happen during the design stage. The actual coding can then be handled by a single developer.
The question is, does pair programming eliminate the need for code review? No!
The two developers doing pair programming are involved in writing the code from the same perspective. They will likely interpret the requirements, think about design, and analyze the problem in a similar way. There’s always a need for an external point of view – someone to ask questions, point out issues, and consider requirements. This is why a formal code review is critical.
Bottom line: don’t treat pair programming as “we had code review already as part of the programming phase itself.”. It can come back to haunt you.
Code review tools
Code review tools can be useful to manage the review lifecycle as well as comments and issues that might arise. Some teams find them beneficial; others prefer using a simple document or opening issues directly in source control. If tool-assisted code review sounds like it would work well for your team, look for a tool that makes it easy to collaborate, track changes, and share comments – for both code review meetings and offline code reviews.
Here is a list of some popular tools:
- SmartBear Collaborator (commercial, link to a free trial)
- Gerrit (open source)
- Codestriker (open source)
- Atlassian Crucible (commercial, link to a free trial).
A positive tone and friendly questions with clear concise and decisive tasks
Make sure to keep a positive tone and friendly questions (as opposed to attacking and insulting questions and tone). Ask as if you are not sure, even if you think you know the answer (“is there a lock for this operation? do we need a lock?” and not: “I don’t see a lock here, you have a bug!”). It’s also okay (and encouraged!) to give good feedback (“nice approach, well done!”).
But when it comes to the tasks resulting from the code review, those should be clear, concise, and decisive (“Need to add a locking mechanism for the update cache operation”).
Type of comments during code review
There are a few types of comments during code review, and each should be treated differently:
- Renaming – it’s okay to discuss names in code review. It’s no secret that naming things is one of the hardest aspects of coding (and there is a good reason for that, or not). And yet, naming is crucial for code readability and maintainability. Don’t be shy to ask for renaming in code review, even if it takes much of the code review and would require setting an additional review (in part two we’ll discuss naming comments and best practices). In many cases, renaming can be done instantly, with the help of the IDE. If this is the case, let the developer handle it. If it requires some more time (to think of a proper name or to refactor other parts of the code beyond the IDE reach), document it as a comment and let the programmer handle it later.
- Documentation – like naming, adding comments where the intention is not clear or improving existing comments is crucial for readability and maintainability. This must be part of your code review. And again, for short comments allow the developer to add them ad-hoc during the code review itself, otherwise document it as a task to be done. Note that adding proper comments should be a showstopper preventing the code from being integrated, as leaving it for later might result in the comments simply being forgotten.
- Not meeting coding guidelines and conventions – coding guidelines and conventions allow us to maintain a big code base and understand code written by others. Code review is the defense against non-standard or non-compliant code getting into your repo.
- Deficiencies, bugs, and potential bugs – not meeting the requirements, missing error handling or not handling a specific edge case, logical errors, bad design, inefficiencies and wrong assumptions – these are the bread and butter of the code review (in the second part of this blog we’ll dive deeper into this, including specific C++ items).
- Needs investigation – during code review some questions won’t get a clear-cut answer (“is this operation being called from two threads simultaneously?”, “does the specification allow the operation to be canceled if we reached this stage?”). The resulting task will be the investigation itself, and in many cases, the action to be taken with each of the possible answers would also be documented in the task.
- Nice to have – in some cases, we have a suggestion that we do not expect the program to implement for this iteration, but we want to document it as a nice thing to have. This may include suggestions for efficiency improvements and code simplifications that are not mandatory or would be too time-consuming. We still want to remember they were raised for two reasons: (a) we may want to implement them if we have the time or the need later, and (b) we want to avoid raising these suggestions again in future reviews (so we document it now). Such comments can go into the code as a TODO comment, with an expiration date (after which the TODO can be deleted if not performed).
- Next time maybe do it differently – these kinds of comments belong to the educational side of the review. In some cases, there is a piece of code that could be written differently, but it isn’t worth it to change currently (it works, it’s okay, don’t touch it). Yet, you may still want to alert the programmer that it could be written in a more elegant or simpler way. If you have time, raise this in the review, even if it is not for a TODO comment in this case.
- Not worth a comment – avoid comments like: “this is a good approach, but it could have been done differently” – when there is no advantage to the alternative. Make sure to avoid unnecessary comments that waste time during the review and divert the focus from the real issues. Some reviewers like to use the code review for heroic stories on how they did similar things in the past. Don’t do that. Talk directly about the code being reviewed and if you do not have something substantial to contribute, don’t feel obligated to share your story.
Where to start the review from?
Reviewing code changes
When reviewing a small new feature or a bug fix, usually the review is focused on the code being changed. In this case, it’s best to review the code side-by-side, comparing the old code to the new. Start the review with the list of files being checked out and explain at a high level the need for change in each. Then dive into the details of the changes in each file, in the proper logical order. If you start with a high-level explanation, you may take the review in any direction, top-to-bottom or bottom-up.
Reviewing new code
Start by explaining the requirements being addressed and the high-level design, then list the new classes and files being added. Make sure to delve into the more complicated parts (new algorithms, concurrency issues, etc.). Review the code in a proper logical order (do not jump directly to the new algorithm if the audience doesn’t have the proper context to understand it). Leave enough time to discuss the more complicated parts of your code (if there are only five minutes left to review the complicated algorithm, just schedule another review meeting, don’t try to squeeze complicated stuff to an insufficient review time). Here again, starting with the high-level explanation should allow conducting the review in any direction, top-to-bottom or bottom-up. Even so, for new code, in most cases after getting the high-level explanation it would be easier and more efficient to follow a top-to-bottom approach.
Of course, a review may include a mixture of new code and code changes, in which case it would be good to use the proper approach for each. The logic of where to start in such cases, from the new code or from the code changes, depends on which order would make it easier to follow and understand.
Reviewing the tests
Going through the unit tests – be it new unit tests or updates to existing ones – should be part of your code review. This is your chance to make sure that the new code is being well tested, and that the tests are covering real scenarios and not invented scenarios that have nothing in common with the requirements. You may want to consider starting the review from the tests, as in some cases this is the best entry point for understanding the feature or fix being implemented.
Summary
This section covered the basics of an effective code review. In the end, a code review is not meant as just another step. You should use it to give your code higher quality and fewer bugs. Conducting meaningful code reviews helps you highlight issues earlier in the process when the code is still fresh in the writer’s mind, who can fix things much easier than later in the process, and before wasting others’ time.
Read more about how to do C++ code reviews more efficiently in part two, here!
