Last time we discussed why the code review process is an essential part of software development from the perspective of the product owner. Now let’s discuss the code review from the dev’s point of view.
“Talk is cheap. Show me the code.”
― Linus Torvalds
We cannot agree more with Torvalds on this. The code reviews are the key component of the developer culture at 2muchcoffee. To do this practice right is especially important to us since delivering robust products is our aim and behind all great products is the clear source code.
In this article, we’d like to share our thoughts on code review best practices and why it’s an important element of a successful team and product. If you don’t do code review, or if you feel like you could do better, we hope this article will help!
How to Conduct a Good Code Review?
Code review is the practice of peer review of the source code. The aim is to find defects in the code in the initial development stage in order to improve the overall quality of software. It is a perfect approach if you want to check junior devs, improve code readability, and share knowledge within the team. So, this our code review guideline.
Rule #1. Know what to look for in a code review
Look through the structure, style, logic, performance, test coverage, design, readability, functionality. While automated checks are great for checking structure and logic, the manual review is suitable for functionality and design.
Reviewing code with certain questions in mind can help you focus on the right things. For instance, you might evaluate code to answer:
Do I understand what the code does?
Does the code function as I expect it to?
Does this code fulfill the project requirements?
Rule #2. Build and Test - before code review
Don't assume the code works - build and test it yourself! You should actually pull down the code and test it out. Of course, when testing code, make sure you're building correctly. If the project has a build system, you should be able to use it. If the Continuous Integration system reported successfully building the code, you should be able to as well. On this note, if the CI build failed, you should require that the code successfully build before it can be landed!
Once you've compiled the code, actually test it.
Rule #3. Commenting matters
At 2muchcoffee, every logical statement should have a comment describing the programmer's intention for it. Assuming you're working on a project that follows this convention if you don't see an intent comment, you should request one to be added to the code. Then look for it before you approve. In regards to comments, it isn't enough just to have something there. Intent comments should actually describe intent.
Rule #4. Give feedback that helps (not hurts)
Constructive feedback is what it’s all about. You can do this by asking questions, rather than making statements. And remember to give praise alongside your constructive feedback. Giving feedback in-person (or even doing your review in-person) will help you communicate with the right tone. The code will always need to be reviewed. And you’ll always need to review your coworkers’ code. When you approach reviews as a learning process, everyone wins.
Rule #5. Consider how the code will work in production
Design is important, and integration matters. How will this code function in the real world? How will it handle bad input and user error? Will it play well with the rest of the codebase? In short, be demanding of the code.
Rule #6. Check documentation, tests, and build files
Good code doesn't just include code, it includes all of the trappings that go with it. At 2muchcoffee, we expect that every revision will contain all of the following:
Tests covering the new code. Review these as strictly as you do the code itself, to ensure the test will fail if there is a problem.
Documentation for the new code. The best documentation is written in tandem with the code itself. Don't accept documentation later; it should be present within the revision itself!
README changes. The markdown files, such as README.md, BUILDING.md, CHANGELOG.md, and so forth should reflect the latest changes. In reality, these rarely need to be changed, but you should be sure they're up-to-date.
Rule #7. Keep priorities straight when making suggestions
The code should be...
Functional first,
Clean and maintainable second, and
Optimized third.
The code should ultimately achieve all three, but the order is important. If the code doesn't work, don't worry about style yet. Similarly, if the code is broken or poorly styled, optimization is only going to make things worse.
Rule #8. Follow up on reviews
After suggesting changes, you should be prepared to review it again. Ensure the necessary changes were made, and any problems you found were reasonably resolved.
Be sure to devote just as much attention to the follow-up review as to the original one! Apply all ten principles anew.
Rule #9. Reviewers are not perfect
Reviewing can be daunting, so it helps to remember that reviewers are not perfect! Issues may slip past you, bugs may evade detection, performance flaws may make it to production...in short, broken code happens!
If you are not familiar with the code or concepts, you may want to request that an additional reviewer provide feedback, but don't shy away from doing the review yourself! Ultimately, four eyes are always better than two.
If you do realize you've made a mistake in a review, the best thing you can do is own up to it. Raise a concern on the post-commit review system if appropriate, or else file an issue/bug report.
Code Review Checklist
Code formatting, architecture, non-functional requirements, object-oriented analysis, and design principles are the main features that guarantee maintainability, reliability, scalability, and usability to your new product development.
The following code review checklist gives the vision of what you need to consider during the code review practice:
The Side Effects Of Code Review
However, there are a few side effects of the code review, which every dev should take into account.
Getting Personal
A common issue with code reviews is that people can sometimes feel personally offended by some of the comments. This isn’t too surprising: code reviews are an opportunity to provide feedback, which often comes in the form of criticism, and criticism is a tricky thing. It’s easy to give criticism in a careless way, and it’s easy to interpret criticism as a personal attack.
Constructive criticism is essential to growth, but in order to be effective, both the giver and receiver need to be thoughtful about their communication.
Tone
It’s also important for commenters to be mindful of tone. For example, consider the following comment: “This is the wrong way to structure the test. There are too many expectations in one example.”
While not directly personal, this type of comment also isn’t likely to be well received. As Hendrie Weisinger explains in The Power of Positive Criticism, this kind of right/wrong distinction threatens an individual’s self-esteem, reducing the likelihood the recipient will be open to the feedback. The comment above could be rephrased as: “I think it would help with readability to split these expectations into separate examples. Here’s a reference:..”.
Another easy way to protect someone’s self-esteem is to ask questions rather than stating opinions. For example, suppose you’re reviewing code and you think: “These 3 lines really belong in a separate method.” Instead of writing that, you could write: “What do you think about extracting these lines into another method to isolate the calculation logic?”
It’s a small tweak, but this style of feedback communicates respect to the recipient and provides the space for a productive discussion.
Wrong Time, Wrong Place
Sometimes code review comments get off track. That’s not to say people start discussing random topics, but more that they’re discussing the code at a level inappropriate to the code review.
Style
For example, how many pull requests need comments debating the appropriate amount of whitespace? Code reviews with long debates about issues of style indicate that the team needs to establish some conventions and agree to stick with them.
Once you have a style guide in place, code review comments may still, of course, include discussions of style issues, but they can include a link to the style guide instead of stating an opinion and opening up a debate each time.
It’s even better when you can integrate an automatic style-checking tool like Hound into your workflow. Then you can minimize the human involvement in style guide enforcement.This will save everyone time and allow for the discussion to focus on the more important and perhaps more subtle issues.
Architecture
On the other end of the problem, if code review comments are too high-level, it might be a sign that a much-needed architecture discussion didn’t take place earlier in the process.
If there are any architectural problems with the code, the reviewer has a duty to call them out, of course. Final code review is just not the optimal place for a sweeping discussion of how to approach a problem. Ideally, these problems are identified in a whiteboarding session before a line of code is written.
Neglect
Another common problem with code reviews can be that people don’t do them. Pull requests sit around for a while waiting for their comments. This can happen for a number of reasons:
Length.The problem is people can feel rightfully overwhelmed by such large changes. It’s really up to the code author to keep changes as small as possible.Of course, occasions arise where you can’t avoid a big change. In those cases, it’s often worth scheduling an in-person code review meeting. It will likely be more productive than a long comment chain.
Presentation.In fact, no matter what the size of your pull request, sometimes it just makes more sense to schedule a code review meeting. It may end up consuming more time, but it’s one way to help elicit the feedback you need.
That said, there are also ways to present your code changes within a code review tool that will help attract reluctant reviewers. Anything you can do to help potential reviewers understand your changes will help. At the very least, you can link to the relevant user story.
Conclusion
Once you get a solid process in place, code reviews become a great opportunity for learning, and the overall code quality improves. Continued success requires a team to remain committed to the process and educate junior devs along the way, but enjoying a healthy code review culture is well worth the effort.
In case, you will have any questions or suggestions, feel free to contact our team and we will assist you in any inquiry!
