Peer code reviews are a great way to catch software defects early and often. This practice has been around since at least the mid-seventies [Fagan '76].
Extreme programming (XP) has a couple of practices built-in to support reviews. Collective Ownership encourages everyone to contribute new ideas to all segments of the project. Pair programming increases software quality without impacting time to deliver. All code to be sent into production is created by two people working together at a single computer.
Having experienced both traditional code reviews, and full-blown pair-programming, I believe pairing to be a very effective and efficient step towards good, clean code.
Yet, I've had team-mates that didn't want to be part of the process, and I've had team leaders who didn't know how to constructively make it work. I have come across some good practices for peer code review to pass on to them. Even though this is written from the point of view of traditional code reviews, I think that the principles can be applied to Agile environments as well. Below are some excerpts I particularly enjoyed:
8. Managers must foster a good code review culture in which finding defects is viewed positively.
Code review can do more for true team building than almost any other technique we’ve seen – but only if managers promote it at a means for learning, growing, and communication. It’s easy to see defects as a bad thing – after all they are mistakes in the code – but fostering a negative attitude towards defects found can sour a whole team, not to mention sabotage the bug-finding process.
9. Beware the “Big Brother” effect.
Managers must continue to foster the idea that finding defects is good, not evil, and that defect density is not correlated with developer ability. Remember to make sure it’s clear to the team that defects, particularly the number of defects introduced by a team member, shouldn’t be shunned and will never be used for performance evaluations.
10. The Ego Effect: Do at least some code review, even if you don’t have time to review it all.
The “Ego Effect” drives developers to write better code because they know that others will be looking at their code and their metrics. And no one wants to be known as the guy who makes all those junior-level mistakes. The Ego Effect drives developers to review their own work carefully before passing it on to others.
Good to see you posting again!
ReplyDeleteBut I have a problem with the article you linked to.
'It’s not a case of “the author made a
defect and the review found it.” It’s more like a very efficient form of pair-programming.'
-- p.5 of the linked article
Not to be too snide, but the most efficient form of pair-programming is ... pair programming.
I think the author of that article seriously does not understand Agile methods. The most egregious example is in item 7, "Verify that defects are actually fixed!". Even if you're not doing TDD, the best way to guarantee that a bug is fixed AND that future changes won't silently resurrect it is to write automated tests for the bug. This should be obvious, but it is not mentioned at all in this section of the article.
The TDD way to fix a bug is to write the test first and see it fail, then fix the code. You now have a regression test.
I've participated in a lot of team-wide code reviews in my time, and - as the author points out - they are boring, annoying and not really productive.
I would consider using code reviews either as a response to bugs being found "late" (e.g., by customers) or as an occasional spot-check: if reviewers do find bugs, the process needs to be fixed. If TDD and pair-programming are not catching problems, they need to be improved. For example: writing big complicated tests instead of taking baby steps.