Developers are sensitive people, we are creatives with an objective task. X program needs Y output, in whatever way that materialises.
We must think abstractly through the multiple layers present in an application, and codebase, in order to come up with the best solution.
And it’s not always easy.
Within this story are some trophies of review experience. These few points will help you become the reviewer everybody wants on their team.
Positive feedback — point out, and praise decisions
When trawling through the web of another developers’ creative decisions, be an ally. Point out the great ideas they’ve introduced, or nifty optimisations along the way. This can be as small as saying:
“Wow I wouldn’t have thought of that!”, to “Great idea, and execution here”.
Us as humans tend to focus on negatives rather than positive, that’s an artefact of our monkey brains focusing on survival. Although it may feel like it at times, code review isn’t a question of survival, it should be constructive criticism to improve the codebase, and it’s maintainers.
So be an ally, don’t let your coworkers focus on negatives, point out the ideas they’ve sown and praise them.
Be constructive, not destructive
One trap that you may fall into when reviewing work is just pointing out something that is bad. But, not explaining why it’s bad. Sometimes it’s painfully obvious as to exactly why you wouldn’t do the same thing, but that doesn’t mean it is to somebody else. And there’s always a chance the way you’ve been doing things isn’t right, or could be improved upon.
When pointing out an error, an issue, or a mistake. Give a clear explanation, or example on how to improve it.
“I’ve noticed you’ve done X here. And perhaps X isn’t the solution, the reason being that….”
“I believe Y may solve the problems you’ve encountered here, and be a cleaner approach”Why are you requesting changes? Is there a clear path to an ‘approved’ review?
Requesting changes can be painful. The work you’ve done, which you believe to be good, and valid, hasn’t passed the mental tests of another developer.
Well you as the reviewer should be explaining clearly why. If there’s no clear path to an approved review, what is the reviewee supposed to do? You’ve now left them frustrated. What is there for them to improve? What haven’t they done right?
Be clear, and concise, whilst wary of the language you’re using. Provide a list, or series to points of what needs to be addressed. E.g.
- X approach could lead to Y problem
- X could be improved upon, as there are scenarios where Y could occur
You’re a part of the solution
The last point I’ll touch upon is that by requesting changes, you’re now inexplicably a part of this PR.
Your approving review is now required for the changes to be merged into the codebase. So make sure you’re available to review changes in a timely manner. We’ve all experienced somebody requesting changes, then not re-reviewing work for an elongated amount of time. All the while leaving you to wait, stuck, needing your changes to be merged in.
Conclusion
What do you think?
What do you think constitutes a bad review?
What makes a good review?
Are you already a code review guru? If so, what are some tips you could pass on?
