Welcome to BSP Software’s ‘Code Review’ series
Part 1) Author and Reviewer Preparation
Part 2) Readibility
Part 3) Structure
Part 4) Algorithmic soundness
Peter Gabris, MD of BSP Software Inc
In this section we focus on
Part 5) Bugs found, now what?
Record the exact place of the issue and note the character and severity of the issue. If you are not sure, ask the author. Remember: a question “What was the reasoning behind the deviation from the standards here…” is better than a question starting with why and much better than a statement. Even when you are positive the thing you see is a bad bug, it might be better to ask question.
There are a couple of things you as a code author can do with reviewer’s email:
- Dodge, avoid or ignore it. This is the worst thing you can do.
- Fix it, if you can. Ask your reviewer or somebody else if you are not sure how to fix the issue.
- Explain to your reviewer the issue is not a defect. If the developer and the reviewer agree it is not an issue, comment the place in the code. If it was hard to understand to your reviewer, it might be mysterious also to the next programmer tasked to modify your code. If the developer and the reviewer can’t decide between themselves if an issue should be fixed or not, then more people has to get involved. Bring in a manager, team leader or another experienced team member to decide.
Never answer your reviewer’s email with inserting your answers into his text; at least if you are neither his boss nor his parent. Leave hers/his message intact. If you need to quote her/him insert the quote into your text in quotation marks (after all, this is what they are for)
Remember: the code review is about the code, never about a person
Share the gain
Frequently the code review reveals a nugget of deeper knowledge. This gain should be shared with the rest of the team. The code author is better suited to break the news. It is a sign of your upbringing to give the code reviewer some credit.
Code is reviewed until it passes
Code isn’t reviewed once and then forgotten. Any changes made have to be re-reviewed. Not reviewing all changes makes the process useless. The code author is responsible for all bugs left in the code. There are always defects that will sneak through the code review and through all tests. Adding to them an issue the code reviewer pointed out is a sin punishable by permanent loss of sleep.
Code Review continues in Part 6) Implementation strategy
Feel free to leave any questions or comments below