“I enjoy doing code reviews!” said no embedded software development engineer, ever.
Nobody in their right mind takes pleasure in combing through hundreds of lines of Consolas 9.5 gibberish they didn’t even add, modify, or remove. There are no rewards. No incentives. Only an engineer at the other end of the diff-viewer who thinks you’re just trying to get under his skin, thinks you’re a know-it-all, or assumes your stylistic preferences are a personal criticism.
Okay, that may be a bit aggressive, but nonetheless, can be very true. The author of the code review has an overly optimistic (yet false) expectation that their code is flawless, while the reviewers (once they drag themselves to actually touch the thing) go into a code review with the intentions of, (you guessed it), finding flaws. If the purpose of code reviews is to make our software better, why do authors have such an issue with defects and constructive criticism during the review?
The truth is; nobody is perfect. If we were, code reviews would not exist, tests would not be written, coding & modeling standards would not be in place, etc. The first mistake in the review process isn’t even the reviewers’ fault, but rather the author’s when they assume their content is flawless when first deployed in the review. The second misassumption is also the author’s when they assume a reviewer’s comment or defect is a personal attack on their “hard work”.
Here are some tips I have gathered when authoring and performing code reviews:
Author:
- Pre-review. Diligently sifting through your code changes in their entirety prior to submitting for review will limit (and hopefully eliminate) any chance of review comments and/or defects.
- Carefully select the reviewers. If reviews only require a subset of your team to approve the review, select reviewers who know the code-base well, will perform the review in a timely manner, and will perform the review as thoroughly as possible. This will not only help your review get approved quicker, but with a higher confidence of quality.
- Follow-up. After submitting the review, follow-up with the reviewers on a consistent (but not annoying) basis. This keeps your review fresh in their daily to-do list so that it doesn’t go stagnant.
- Offer a code “walkthrough” if appropriate. Some code changes are just downright complicated, and only make sense to the author. Code reviews of this stature may be driven to approval much quicker when all necessary reviewers and the author spend however long it takes to explain every deletion, modification, addition, etc. that was made instead of each reviewer brainstorming by themselves why each change was done a certain way.
- You’re not perfect. Your reviews WILL get comments and defects; it’s a fact of life. The beauty is… they exist to (potentially) make your software BETTER, help you gain understanding, and to facilitate knowledge-sharing. Many times these elements are viewed as harsh attacks instead of constructive criticisms as they should.
Reviewer:
- Be timely. When a code review is first submitted, continue to the next convenient stopping point in your current work, and be intentional about completing the code review immediately after. There is no “I don’t have to do it, because other people will” mentality in the realm of code reviews. The sooner you can get to the code review, the sooner it will be over with and won’t have to think about it.
- Back up comments and defects with purpose and reasoning. No author appreciates “change X to Y” critiques, but may be more accepting of “if you change X to Y, Z will be more efficient and readable”. Not only will this approach help the author know the purpose of your suggested change, they will be more likely to make the change, and this facilitates greater skills development and knowledge sharing… with the biggest payoff being not making the same mistake in the next code review.
- Look for code that did NOT change. Remember: it’s not just what changed or what’s new that really matters, it’s 1) was any existing functionality removed that was not intended, 2) does the new code meet the new requirements and ONLY the new requirements, 3) what DID NOT change that was supposed to, and 4) were any threats to quality, safety, security, etc. introduced as part of the change. Too often reviewers are sucked into the trap thinking only code the author changed, added, or deleted need to be looked at (only what the diff-viewer shows them), when the truth is that code the author did NOT change, add, or delete need extra-special scrutiny.
The next time you’re signed up for a code review you really don’t want to do, or submit a review and hope the minefield doesn’t rear its ugly head, remember that code reviews are not as bad as they’re made to be. Only an abundance of saved debugging time, knowledge sharing, skills developing, and solid, quality software can be attained by such a routine task when performed well.
