Application developers often have a responsibility to review the program code written by other developers as part of their development process. While my organization refers to this process as a subset of a Pull Request (PR), most organizations I've worked with refer to this process as a code review. The goal of this article to focus on what really matters during this peer review process.
Why Have Code Reviews?
I think it is important to take a step back and remember why code reviews are part of the process. Below, are three quick reasons why having a peer review of your code, before it is merged into a main branch, is a beneficial step in the process:
Avoid Introducing Bugs/Issues — Another set of eyes can help identify some potential bugs right away, especially since the focus is on a subset of the code.
Design Suggestions — Often developers get focused on their unit of work, that they may end up off track from design standards that exist. A code review often reveals this scenario.
Learning Opportunities — This actually goes both ways. First, the reviewer could point out a better way to handle the situation, or the developer could train others on a new approach.
What's Bad About Code Reviews?
Having a code review process in place does have some not-so-positive side effects:
Time — The process consumes time from developers who are not writing code when they are performing a code review. This can get really crazy near the end of sprint cycles, when several developers want to get their code reviewed on the final day of the sprint.
Not Making It Personal — Taking criticism is not easy for most people and that is no exception with code reviews. Basically, you are putting your code under a microscope for everyone in the review process to see and comment on. So, it is hard not to take it personal when a change is requested. An important key is to make sure any comments or suggestions made are constructive in nature.
In the grand scheme of things, I believe the time element turns into a cost savings, when factoring in the consequences of not having code reviews in place. The whole personal aspect is something that comes with time and experience.
What Do We See First?
As code reviewers, we are human and our mind naturally gravitates to looking at the following things initially during a code review:
Formatting or Style of Coding — How is the code indented, what method or class names are being introduced, and how the code is organized are all examples that most reviewers tend to focus on first.
Documentation and Comments — Referring mostly to the non-code elements contained in the source under review.
There are a wide variety of code style checkers and linters that can be added to either your build process or IDE itself, to help handle some of these items. To me, these items are not as important as the next section (below), except to remember to make sure your comments are accurate - as I discussed in my Code Commenting Patterns article.
What Should We See First?
The real value for the code review is when the reviewer places a focus on the following items:
The Reason Behind the Code — In order to review the code, the reviewer should understand the reason behind the code. With that understanding, it puts the reviewer in a better position to review the suggested approach.
Complexity — The reviewer should validate if the approach is the *right* approach, making sure to keep from being over complicated.
Ease of Understanding — Keeping in mind that others will likely need to update and enhance the code, reviewers should make sure the code is easy to understand and broken down in sensible units.
Been There, Done That — The reviewer should make sure this problem hasn't already been solved before.
Show Me the Unit Tests — If your organization implements unit tests, make sure the unit tests are in place - with a valid coverage percentage.
Learning Opportunity — Similar to what was noted above. If a common problem continues to reveal itself - it might be time for a lunch and learn with the entire development team. At the same time, if something new is being introduced, a lunch and learn could be used to introduce the concept to everyone.
Security Impacts — Most importantly, the reviewer should have their "security hat" on, to avoid introducing any security risks in the code base.
The code review process is something I place a great deal a value in, based upon my background and experience. In my personal experience, developers were initially worried about the time element to perform the code reviews, but we quickly saw a reduction in minor bugs that were identified by the code review process.
Have a really great day!