A code review is having someone other than the author check someone else’s code for errors or mistakes. Code Reviews are a great time to share knowledge, and learn from one another. I find that often code reviews have a lot of really common mistakes that make them a less effective.
In a previous blog post, I blogged about How to do Code Reviews. For those not aware, a code review is a key way for developers to find bugs, and share knowledge. Often, the code reviews don’t really accomplish either.
I find that the reviews fall into one or two groups of errors: no feedback or a long list of changes explained inefficiently. By finding bugs earlier in the process, we’re able to reduce quite a bit of the development cost because bugs cost a lot more once they are discovered in production and impact users. Code Reviews can be formal or they can be informal, I’ve had a lot of experience with both over the years.
One of the best places I worked had the technology manager, review a lot of the pull requests and then go through them with the developer. It wasn’t very formal, but we sat at his computer while he commented about the changes. In the year or so I was there, I learned so much and feel like it made such a difference in my life.
Since then, most of the places I worked had very little to no feedback and I feel like that was a huge missed opportunity for the companies and for myself.
No Useful Feedback
it’s really important to split work into really small pieces so that everyone can better control and allow the review to be far more effective. A good sized change set would be a few files and maybe a couple hundred lines of code added/changed.
A change set with too few lines can result in one or two things happening: either the developer is really nit-picky and has a lot of changes that have to be made or the developer doesn’t give any feedback at all.
When the change set is too large, the result is usually that the developer or manager reviewing it loses focus and doesn’t provide any feedback or doesn’t catch any bugs.
Wasting reviewers’ time
Looking at syntax and style is generally a huge waste of time for the reviewers and the reviewed. It makes a lot more sense to use a tool like a linter to enforce consistent standards. In some languages and IDEs there’s ways to automatically fix a lot of these inconsistencies. ESLint and JavaScript Standard Style are pretty good examples of tools for JavaScript.
A properly built continuous integration / continuous delivery pipeline has the potential for this to stop happening. I like to have prettier automatically format my code just before it’s checked in. In VSCode it’s possible to set this up on file save.
The reviewer really has a responsibility to take the time to review and understand why changes are being made. What was the problem that the developer was trying to solve.
Doing a code review without a checklist
I like to perform my code reviews with a checklist. The checklist I use is really basic but it’s worked pretty well for me.
I like to pay attention code security, business logic, user access, and finally does the code fix the problem that needs to be solved. I like to have a list of common mistakes that my team makes and check for those errors every time I review.
People taking too little time to do the code review.
Code reveiw should be a great time to confront potential issues before they make it into production. Time for dialog has to be made as part of the review. It’s really important that there’s enough time for good discussion and potential solutions to be developed.
If you can’t do a complete code review, it might be acceptable to only review 30 – 50% of the code. I wouldn’t personally do this without taking some time to review the tests and making sure they cover the majority of the code being changed but it’s an option.
Scheduling the code review 30 minutes before the demo needs to happen won’t be very effective.
