Wikipedia says Code Review is:
systematic examination (often as peer review) of computer source code intended to find and fix mistakes overlooked in the initial development phase, improving both the overall quality of software and the developers' skills.
I like this definition, especially the part about improving the developers' skills. I think you really benefit from having your code reviewed by an expert in that particular area or a more experienced developer, but I digress. Actually I'd like to know how code reviews fit into the agile world.
Let's take a look at Google's process for managing changes:
- author edits changes in workspace & tests
- author sends email to reviewer
- reviewer views diff
- reviewer sends email back
- possible email discussion
- when reviewer gives it's OK (lgtm) authors submits changes
There are two things, which I think are very interesting about this process. First off, in terms of agile development, Google has in some sense implemented a strong Code Ownership model. On the one hand you are allowed to make changes to any class or module. However, before those changes actually go into the global code base, they need to be reviewed by the original author - or therefore owner. That's exactly what I think is the best model of code ownership. The second issue, which I find particularly interesting is the strong workflow of the process. Google tries to ensure quality, by forcing a review before any changes go live and - at the same time - improving the skills of their developers.
I like this processes a lot and I'm asking myself how could I integrate this with Scrum? Unfortunately I couldn't find much information on agile development and code reviews in general. Wikipedia brings up Extreme Programming and their own way of code review: Pair Programming. I prefer Google's way of implementing reviews - I'm not a big fan of pair programming.
There was a discussion on the Java Posse Google Group a couple of days ago, where someone suggested an explicit review task per sprint. Only reviewing a big junk of code at the end of a sprint is not only too late, but also no fun for the reviewer. In my opinion, the notion of a discussion and the (almost) immediate feedback by a co-worker is the most valuable part of a code review. I might be wrong, but I guess putting it off into a separate task is not ideal.
So where do code reviews fit in? If I'm not wrong in the world of Scrum, a task is finished if the code is ready to be release. That also means it has been tested and documented and thus testing and writing documentation are both part of the task. Why not make a code review part of the task? In fact, I think it should be the next step after your test passed and your code is ready to be checked in. I wonder if this could work.
Google's process is supported by their homegrown tool Rietveld, which is also available as OS version. It handles diffs, let's you annotate code with your comment and sends emails back and forth between the reviewer and the author. We have Atlassian's crucible at work, which basically works the same way. I like the tool a lot and I'm planning on using it extensively, as soon as the process of when and how to do code reviews is defined. I think it's important to have a good tool, that assists you throughout the review process. Nobody wants to scan through ugly unified diff files in order to copy and past code snippets into emails. It's just so much more convenient to use a tool.
I really wonder why there is so little information about code reviews in the agile world. It almost seems like there is no place or time for it (except for the crippled version used in XP), when you are doing agile. Maybe people are afraid of blaming each other. It might be tough for some developers to speak their minds, if they think something is wrong with the code the are looking at. There might also be the notion that it is a tool to blame other people, which I think the opposite is the case: you are helping out other developers to improve their skills. But you could also see it as a tool to make lazy developers think, before they check-in their code.
4 comments:
I agree that code reviews are an extremely powerful tool to ensure quality. However, it's extremely difficult to get them right and a lot of people got burned. If you fail to create a constructive environment where learning and improvement is key then you'll have some serious social problems at hand. Some people will quickly feel insulted and others will no longer be honest and won't mention problems any more.
I think this will happen quickly if you miss the first precondition to code reviews: Objective quality standards like coding guidelines and other metrics. Unless you have them, don't bother with reviews or you'll end up fighting over trivial things like indentation.
In my opinion, pair programming is a pretty good alternative to classic code reviews. All happens in real time and your code is created as a (micro) team effort instead of written by one person and then scrutinized by another. It works well with centralized revision control and you don't need any other review tools (or even distributed revision control systems).
I've done pair programming a few times and it always was a straining experience. Both programmers need to be fully concentrated all the time, you don't take as many breaks as when you're working alone etc. I often paired with a very good developer for the most difficult and critical refactorings and the results were always worth the effort.
Thanks for your input Matthias!
unmaintainable> If you fail to create a constructive environment where learning and improvement is key then you'll have some serious social problems at hand.
I concur! It's really crucial to establish a constructive and objective sentiment when raising criticism.
unmaintainable> Objective quality standards like coding guidelines and other metrics. Unless you have them, don't bother with reviews or you'll end up fighting over trivial things like indentation.
I see what you mean! Thanks for pointing that out!
unmaintainable> I often paired with a very good developer for the most difficult and critical refactorings and the results were always worth the effort.
How I envy you! I guess that's the advantage of working in a big company...
I have been using pair programming for a couple of years now, mainly because we have inherited a lot of technical debt in the form of legacy code from inexperienced developers where no reviews at all had taken place! My experience suggests that pairing should be done in 98% of any development, its fine to work alone on low risk areas of functionality but elsewhere it is highly desirable to have a detailed review.. I posed the question to a colleague, if I sit down with him after he has spent 6 hours and wrote 100 lines of code how likely is he to throw it all away if I review it and recommend a rewrite..the answer was not very likely, so I asked the question what about after 50 lines of code, the answer was the same with a maybe appended!! I then asked what about after 1-2 lines? the answer Yes of course we could discuss this and move on.. So I do think pairing is worth doing as it is not simply checking code, its doing design, considering tests, making sure you are in a position to be "done done".. I admit it can be hard to see the justification at times but in my experience it pays off bug time in quality and minimal technical debt.
Wow... very good article!
I wholeheartedly concur on the importance of code reviews.
My first S/W Engineering internship was at a company where they gave me a (fairly big) project, and had our code review at the very end of developing this big complex system...
a) So glad we actually reviewed the code before sending it off for test
b) OMG, we should have been doing this all along, right from the beginning!
Post a Comment