First, the team should agree on the goal of the code review (you should not take such agreement for granted, as code review can be used for a variety of things, and different organisations do it for different reasons. For this answer, I will assume that the goal is to improve a developer's future work.
To improve, the developer needs to know
- what was good? -> I should continue doing that
- what was bad? -> I should change that
- and why? -> So I can assess the severity of the issue, as well as recognize similar issues in the future
What action is this about?
Feedback should be about a behavior, not a person. Therefore, it should identify (preferably with examples) the behavior the feedback is about. So don't say:
because that isn't about behaviour, but (perceived) personality. Also, it's too vague: Is this about the long lunch break? Slow typing? Not answering email in a timely manner? Too few implemented features? Instead, you should say:
Last week, you were assigned bug #1059. You have marked this defect resolved, but the bug still exists, and can be reproduced with the instructions provided in the trouble ticket.
Next, you should describe why that action or code was good or bad. The reason given should be objective, and of sufficient importance to justify a change. For instance:
AClass.amethod() is invoked by concurrent threads, but not thread safe. Data corruption is likely.
This method name is misleading. It sounds as if we're merely calulating the pay grade, but actually also pays the employee. Somebody not familiar with the implementation is likely to misuse the method, causing accidental salary payments ...
Of course, sometimes you don't have an ironclad argument why the code is bad (or good). This means you don't know you're right. Therefore it is prudent to be inquisitive, rather than presumptious:
This looks awfully complex. Why don't you just invoke AClass.amethod() instead?
This appears to be nearly identical to AClass.amethod(). Why can we not reuse that method?
You can also give positive feedback, of course:
That's a great approach to coordinate the worker threads! I usually do X instead, and I always wondered how I could make it more robust. Now I know :-)
Please don't do this
To illustrate the importance of the above, let's consider a real world counter example taken verbatim from a formal code review:
The code leaves a poorly thought out impression.
And that's all the reviewer wrote about that finding.
That is terrible feedback on so many levels:
- The feedback implies something unflattering about the author (namely, that he thinks poorly)
- The feedback is about an impression. That is not objective.
- The feedback does not identify any part of the code that needs to be changed,
- nor give any reason why it should be.
The recipient of such "feedback" can not know what he needs to change, nor why, and thus not learn anything. In fact, he can not even know that the feedback is fair, and, given the unflattering unsubstantiated conclusion, is far more likely to see this as a personal attack by somebody who wants to elevate himself by pushing his peers down. Shouting is likely to ensue.