<< Previous Article: 'The Principles of Code Review'
For years, I looked forward to reviewing someone else's code. I looked forward to pulling out the scalpel and dissecting the lines of their code to find a better algorithm, to find a better structure.
For years, some of my code reviews descended into arguments over whose approach was better. For years, most of these arguments ended with the code unchanged, the developer ever more convinced of how correct his code was, and further convinced that I was a fool. I'm sure he thought that. After all, that's what I thought of him.
For years, when someone reviewed my code, I would behave the same, too. I would defend my code, I would justify its existence, like a defendant making a submission to the court. And, more often than not, I would stick to my guns. I still do. I can't help it.
The mistake I made when doing code reviews was a basic one: I assumed that the quality of my code review was also the quality of my expertise. If my review was bad, and missed things, then that reflected poorly on me, not on the coder. Which is why I was precise and nit-picky. I didn't want to miss anything and look stupid.
Anything that looked unusual to me was pointed out, and I tried to make it less unusual. Which meant instructing them in how to code in my style, because that's what I was most familiar with.
That's all fine, except for one thing: a code review isn't about me. It's about the code. My job as a code reviewer is to make sure they have written something that accomplishes the job. Not something that accomplishes it in the way I think it should.
Leaving your ego at the door isn't going to help you make a wiser code review. But it will stop you from trying to convince someone that your approach is better. If they disagree at first, that's their problem, not yours. There are few times when good developers disagree with a recommendation that is obviously better. People disagree when the improvement is more a matter of opinion. If you come to the table with your ego, many of your recommendations will be loaded with your point of view, not your technical knowledge.
Your job as a code reviewer is simple:
- Offer factual observations, not opinion of style. Observe possible division by zero errors, not better organization of the division algorithm.
- Don't get bogged down on better names of variables (unless everything is named $a, $ab, $iii ...). Don't get bogged down indentation. Don't get bogged down on brackets. Don't get bogged down switches vs conditionals. A code review is not the place for style reviews. A code review is the place for making sure it does what it's supposed to do, in a way that's maintainable. End of story.
- Don't reinvent their approach. If they way they coded it doesn't violate basic principles of coding, don't call them on it.
- Ask yourself one simple question: is my suggestion based upon my opinion of how it should be written and, if so, why is it better than his opinion?
When you review the code, keep your eye on the ball: you are making sure the code is correct, not that it does it in the way you would do it yourself.
Next: The Second Principle of Code Review: Understanding the Code
No comments:
Post a Comment