Code reviews are an important part of the software development process. They help ensure that code meets certain standards and best practices, and they can also help improve code quality by catching errors early on. However, code reviews can also be a source of frustration for developers if they’re not done correctly.
As a code reviewer, your job is to help make the code better. This means providing clear and concise feedback that helps the developer understand what works well and what needs improvement. A mindful, conscious approach to code reviews can yield incredible dividends down the road as you build not only a solid, reliable codebase; but strong relationships of trust with your fellow contributors.
Here are some initial guidelines or best practices for performing a great code review:
- Read the code thoroughly before commenting. This will help you get a better understanding of what the code is supposed to do and how it works.
- Be specific in your comments. If there’s something you don’t like, explain why. Simply saying “this doesn’t look right” or “I don’t like this” isn’t helpful.
- Offer suggestions for how to improve the code. If you have a suggestion for how something could be done differently, provide details about the suggestion and even some links and other material to back it up.
- Be respectful. Remember that the code you’re reviewing is someone else’s work. Criticizing someone’s work can be difficult to hear, so try to be constructive with your feedback. Respectful, polite, positive feedback will go a long way in making code review a positive experience for everyone involved.
- Thank the developer for their work. Code reviews can be tough, so make sure to thank the developer for their efforts.
Following these practices will help ensure that code reviews are a positive experience for both you and the developer whose code you’re reviewing.
In addition to these, here are a few specifics I look for when performing a code review:
0. Does the code actually solve the problem at hand?
Sometimes when I get into code reviews I forget to check if the written software meets the requirements set forth. As you do your initial read-through of the code, this should be your primary focus. If the code does not solve the problem properly or flat out misses the mark, the rest of the code review is pointless as much of it will likely be rewritten. There’s nothing worse than spending an hour reviewing code only to find out later that it doesn’t work. So save yourself the headache and make sure the code compiles and does what it’s supposed to do before you start.
1. Is the code well written and easy to read? Does the code adhere to the company’s code style guide?
It’s important to format code consistently so that code reviews are easier to perform. Utilizing standard tools to format code can help ensure that code is formatted consistently. Additionally, the code should be reviewed according to a standard set of guidelines. Many formatters will format the code per a chosen (or configured) style, handling all the white space for you. Other stylistic aspects to look for are naming conventions on variables and functions, the casing of names, and proper usage of standard types.
Structural and organizational standards are important as well. For example, checking for the use of global variables, const-correctness, file name conventions, etc. are all things to look out for and address at the time of the code review.
2. Is the code well organized?
Well-organized code is very subjective, but it is still something to look at. Is the structure easy to follow? As a potential maintainer of this code, are you able to find declarations and definitions where you would expect them? Is the module part of one monolithic file or broken down into digestible pieces that are built together?
In addition, be sure to look out for adequate commenting in the code. Comments should explain what the code does, why it does it, and how it works, especially to explain anything tricky or out of the ordinary. Be on the lookout for spelling errors as well because a well-commented codebase rife with spelling errors looks unprofessional.
3. Is the code covered by tests? Are all edge cases covered? What about integration testing?
Anytime a new module is submitted for review, one of the first things I look for are unit tests. Without a unit test, I almost always reject the merge/pull request because I know that at some point down the road a small, insignificant change will lead to a broken module that cascades through countless applications. A simple unit test that checks the basic functionality of the module, striving for 100% coverage, can save so much time and money in the long term. Edge cases are tricky, but if you think outside the box and ensure that the unit test checks even the “impossible” scenarios, you’ll be in good shape.
Integration tests are a different matter. In my line of work, integration testing must be done with hardware-in-the-loop and that quickly becomes cost-prohibitive. However, as integration tests are developed and a test procedure is in place, any and all integration tests must be performed before a change will be accepted; especially if the integration test was modified in the change!
4. Are there any code smells?
Common code smells I look out for are:
- Code bloat: long functions (> 100 lines), huge classes
- Dispensable code: duplication (what I look for the most), stray comments, unused classes, dead code, etc.
- Complexity: cyclomatic complexity greater than 7 for a function is prime fodder for refactoring
- Large switch statements: could you refactor to polymorphic classes and let the type decide what to do?
Many other smells exist – too many to check in detail with each code review. For a great deep-dive I refer you to the Refactoring Guru. Many static analysis tools will check for various code smells for you, so be sure to check the reports from your tools.
The presence of a code smell does not mean that the code must be changed. In some cases, a refactor would lead to more complex or confusing code. However, checking for various smells and flagging them can lead to discussions with the developer and produce a much better product in the end!
5. Would you be happy to maintain this code yourself?
One of the last things I check for during a code review is whether I would be okay to maintain this code on my own in the future. If the answer is no, then that turns into specific feedback to the developer on why that is the case. Maybe the structure is unclear, the general approach is questionable, or there are so many smells that it makes me nervous. Typically in cases like this, I find it best to give the developer a call (or talk with them in person) and discuss my misgivings. An open, honest discussion about the potential issues often leads to added clarity for me (and it’s no longer an issue) or added clarity for them and they can go fix the problem.
These are just a few of the specific things I look for, but following the practices above will help make code reviews a positive experience for everyone involved. What are some best practices or tips you have found to lead to a good code review? Thanks for reading!
Comments