Ask HN: How do you conduct effective code reviews?
Please share any tips, insights from doing so regularly.
- The idea of different types of comment. Suggestions vs questions vs more serious, “I think this might break X”
- As a submitter, take the time to do screenshots if it is UI focused. Read through it once and if some bits require explanation then add your own comments. Basically due diligence
- Be hyper aware of how language can escalate a situation. Ditch abrasive sentences. Re-write to be more questioning or helpful.
- if it’s a naming problem, make suggestions. “I find the name X confusing because Y. How about something along the lines of A or B”
- If it’s a larger problem, and you can meet face to face. Then do so. It will save a lot of back and forth. Or make a phone call. You’re there to make the cosebase better for everyone. Not a public flame war.
- Agree as a team etiquette. Ideally codify it
- Ask yourself the question: “Does this really matter?” Sometimes I don’t bother commenting because while I disagree, I realize that it’s a style thing and it doesn’t affect the codebase at large. Sometimes it’s also worth commenting anyway. That way there is a potential to steer things “your way” but no big deal if not.
- Make positive comments. Try not to dish out praise. If you see something cool, a neat trick or a really elegant solution or even something you didn’t know, then it’s worth writing
- It’s okay not to comment at all. If you can’t find fault. Then just green light. Too many people feel they have to find “something”
- I find it useful to acknowledge when comments have been addressed. This can simply be “Done”.
- The offer to pair on a solution to a problem can be effective
I was going to put this and more into a blog post but have never got round to it
1 - First of all remember these two things: "There is no perfect code" and "There is a human, who doesn't know everything, on the other side". 2 - Understand the scope of the Code. What is this piece trying to achieve? 3 - Read. Calmly. 4 - Read. Calmly. Again. Reading code is hard and takes practice. 5 - If you think something could be done better but it is not required, open the discussion, not everything is a 'change request'. 6 - If you don't understand something, ask. 7 - It's a team play. 8 - Praise what needs to be praised. 9 - Read the tests, for god sake. Try to think test cases that the PR didn't cover. 10 - Understand the moment that the project is. Sometimes some changes are too big for this review. If this is the case, could it be done later?
I know these are pretty simple insights, and no real technique, but they are often forgotten :/
Other than that, have clear standards regarding formatting (preferably do it automatically with something like clang-format), build and run a linter/tests on CI so that the most basic things are caught automatically.
Finally, remember the objective is not criticizing someone else's style, since we all have our own, but instead sharing knowledge and improving quality.