Code reviews are probably as close as a programmer gets to an artist putting their art on display for everyone to judge. It can be nerve wracking having people pick over your hard work, specifically to deem it worthy or not worthy. Talk to any Senior developer and they’ll probably tell you that a good chunk of their time is spent performing code reviews for their peers.
I’ve worked on teams that do no code reviews and I’ve worked on teams that bog down pull requests for two months over opinionated nonsense. The question is, how much time should your team be spending on code reviews and how much of a return are you receiving on that investment?
The Nitpicker
Perhaps you know this person, or perhaps you are this person. They are the one that kicks back an urgent pull request because they don’t like the way you wrapped your if statement. They are also the ones that mire down the entire release cycle because their reviews are so meticulous and granular that the back-and-forth communication alone takes days. Any way you swing it, the amount of effort it takes to nitpick a pull request is likely too much.
So what constitutes nitpicking? Some completely acceptable conditions to look for when reviewing are:
- Your team has taken the time to define a style guide, so submitted code should adhere to that style guide.
- Your team has agreed to full test coverage for your software, so the code should always include full test coverage and not create failures in existing tests.
- The code should be functionally complete and tested on an environment that is as close to production as possible.
- The code should adhere to good programming principles like DRY, decoupling, performant, etc.
- Your code should have appropriate commenting.
For the most part, anything beyond this list is a candidate for being a nitpick for the majority of software. Obviously if you’re writing software for the Mars Rover you may want to get a little more granular.
The Gloss-over
This brings us to our next perpetrator. The Gloss-over! This person either hates or is too busy to properly do code reviews. They take the time to look over the code but apply no critical thinking during the process. The worst part about the gloss-over is that no value is added to the contribution but time was still taken to review it. Of course time is money, so this person is essentially wasting the businesses money without generating value.
Realisticity we were all probably both a nitpicker and a gloss-over at one time or another. There are times where learning to accept a colleague’s contributions that deviate from your own standards can be difficult. There are also times where reading through fifteen commit diffs feels like torture.
Diminishing returns
I absolutely believe that good code reviews can help make good software. I also think skipping code reviews entirely is leaving out an integral part of software development. With that said, I think that code reviews have diminishing returns.
Most team members have a good idea of what is an acceptable submission. This is the origin of concepts like style guides and contribution documentation. Be a professional, take the time to understand the expectations for your team’s code base. If no expectations exist, take the initiative to implement them.
Once those expectations are understood, your code should adhere to them. As for reviewers, use those expectations to navigate your reviews. If there are genuine concerns with the submitted code, by all means submit a change request.
Right down the middle
In my experience, teams that had strong expectations outlined and avoided both nitpicking and glossing over had the best code stability to productivity ratio. These were also my favorite types of teams to work with. Everyone knows what’s expected of them and deviations from those expectations are brought to light during code review.
At the end of the day you should always be able to rely on your teammates to help you become a better developer while also contributing together to make your software better.
Thanks for reading!