Code review – be specific

Posted by

0

self.UpdateMemberDetails = function( memberDetails) {

};

Code reviewer: “are you not following best practices?”

  • There is a sneer behind this comment.
  • Aims to put the reviewee into a defensive mode.
  • It does not describe what is wrong with this line of code; nor does it suggest a best practice to follow.
  • Reviewer obviously has a practice in mind.  But instead of talking about that practice, she moves on to the programmer’s habits, attitudes towards code.

What have I learnt about code reviews from this anti-pattern?

  • Avoid anything that insults or puts the reviewee into a defensive mode.
  • Describe what is wrong and suggest a specific solution, or two.
  • Do not make any assumptions or conjectures about the programmer’s attitude or skills.
  • Ask yourself: What value is my comment adding here?

Good examples,

 if (service.MemberLocation > 3 && service.IsActive && service.Amount > 0 && service.MemberServiceHistoryDetail.DueDate < dueDate)

Code Reviewer: Please format lengthy if-conditions by placing each term of the condition on a separate line.  The code that spills beyond the edge of the screen here is critical to the understanding of the business logic.  For example,

 if (service.MemberLocation > 3 &&
service.IsActive &&
service.Amount > 0 &&
service.MemberServiceHistoryDetail.DueDate < dueDate)

Leave a comment