Code Review Mashup
code reviews, software development
We ran what I’m going to refer to as a code review mashup early this week on a project. The preparation looked like:
- Setup a meeting time for a 2 hour block by the tech lead.
- Setup the code review in Crucible selecting four classes written by three different developers including the lead about one day ahead of time.
- Added the three developers and myself as reviewers.
- Planned to walk through the code on an overhead and do more of an inspection.
- The morning of the review I took about 45 minutes to review 2.2 classes out of four and added about 15 comments to the code using Crucible.
I led/facilitated the review by going through all of my comments. Since I was the only one who had spent any real time prepping, the other developers only caught a few extra things. I think the big takeaway is everyone needs to review the code ahead of time. The last two reviews I’ve been in too much of a lead role, and not spending the bulk of my day coding I miss things other people can catch.
The review meeting ran much more like a code inspection or walk-through, essentially a code review inspection mashup. I’m not sure mixing the two styles really works all that effectively.
Still for about 8 hours of investment across four people we caught:
- A decent amount of coding convention misses, mostly naming conventions such as:
findBySSNneeds to befindBySsn - A design debate over whether to break up a fairly large JSF backing bean which is handling 5 physical pages into five separate backing beans or keep it in one. (We didn’t resolve this for now, but it is a future topic to consider.)
- Whether to use DTOs when using Hibernate or just use the Hibernate mapped model classes. We talked about the potential ease of not using DTOs, and the potential extra coupling of using Hibernate mapped models through the application layers. For now we’re using the DTOs.
Ed Gibbs @ July 4, 2006
In my experience, you can improve the effectiveness of your review activity if you treat it as a professional mentoring activity.
This approach requires some changes comparing to “the normal” peer review process, but the results are much more effective (especially when the reviews are conducted on a weekly basis).
Mainly, more issues are identified, they are fixed almost in real-time with little overhead, and the developers’ experience is being built in a guided manner.
See:
http://blog.qualityaspect.com/2006/04/05/are-your-code-reviews-effective/
Lidor
Lidor’s approach to code reviews is excellent. When you need after-the-fact code reviews, his rational approach to the whole matter sounds very practical and useful.
It strikes me that the use of after-the-fact code reviews on a project may be a “process smell” if you believe you’re using agile development methods. Consider the everyday dynamics of a team that is using XP fully and properly. All the issues you listed that came out of your code review would have been resolved naturally through direct collaboration during development, and would not have come up during an after-the-fact code review.
The “smell” is that there existed any problems on the level of coding conventions or basic design decisions once the team believed it had built and tested the code and it was ready for review.
As with any “smell”, this doesn’t automatically mean anything is wrong, but only suggests you might want to find out where the smell is coming from. If the team believes it is using XP effectively, then some mentoring and guidance is called for. If the team isn’t trying to apply XP “to the max” then it may not be a problem at all; they may be as agile as they are able to be for the moment, given their own backgrounds, organizational realities, and other factors.
In the latter case, Lidor’s approach to code reviews would be a very good idea. I especially like his idea of using the reviews to mentor the developers. I can see how you might use that approach to bring the developers to the point that they don’t actually need (as much) after-the-fact review.
Hi Dave,
Thanks for the feedback. I would like to point out, however, that I don’t consider my approach as after-the-fact reviews.
When done on a weekly basis one might call it “almost real-time” reviews.
The delay between the time code is written in the time it is reviewed is approximately 1 week. In most projects this is more than reasonable trade-off between letting the developer do independent work and still reviewing the code on the time to enable prompt and cost-effective fixes.
The mentoring aspect of professional code reviews is also affected by this time interval. I believe the best approach to mentoring is to enable the mentee some reasonable space to express himself. At the same time the goal of this process is to improve the skills and enrich the experience of the developer based on his own work - the work he did during the week.
Lidor
Lidor,
I see your point. I meant nothing disparaging. In fact, I think what you have described is great.
But the fact there’s any delay at all, no matter how short, between the time code is written and the time it’s reviewed makes the review “after-the-fact.” The “fact” it’s “after” is simply that the developers believe they have finished, and their work is ready to be reviewed.
A one week delay may be fine in many projects, depending on the methodology in use. My comment was strictly in the context of agile projects. If you’re doing, say, two-week iterations, then a one-week delay is significant, whether it’s for a code review or delivery of a server or granting access rights to a new team member or getting the customer to show up for an IPM or anything else.
In the specific context of the agile working style (or maybe more specifically XP), the ideal is to use TDD rigorously and to do pair programming, along with the rest of the “good practices”. With those two practices in place (possibly bolstered by automated code inspection upon check-in), problems at the very basic level of (for example) compliance with naming conventions don’t live long enough to see a code review. Individuals and pairs notice those things preemptively. This is a benefit of using practices like TDD and pairing.
This allows formal reviews to focus on more substantive matters at larger scope than the individual stories, such as integration across layers and so forth. That’s yet another value-add of agile methods.
That said, I fully understand that not every team and not every organization (and not every individual developer) is “up for” full-bore XP. It’s a demanding and intense way to work, and to date only a small minority of teams actually work that way. The necessity for code reviews is all the greater in that case, and the way you handle those reviews sounds excellent.
But I still think it’s worthwhile to keep striving for the ideal…to get those minor problems solved before review time, before the developers are satisfied with themselves and their own results. To a certain extent that’s a cultural change as well as a methodological one.
Thanks for the ideas about doing weekly code reviews and looking more at the mentoring aspect.
I’m trying to drive for weekly code reviews on our projects now, but I’m still the main driver presently and sometimes other management tasks get priority. (Lately I’ve been getting more direct requests from the CIO, not something you want to ignore.) Crucible even as a beta makes this process easier, but I need to get the developers sold a little better on the process.
I have noticed the mentoring aspect as it is an obvious place for design discussions to arise. Often these are real design decisions that have to be made, but the developer wants to know if they’ve chosen the ‘right’ path. One current design decision is using Hibernate mapped model classes up through the controller layer or mapping everything to generic DTOs to avoid the coupling but add a layer of conversion.
Finally, on XP. I’d really like to try full-bore XP, but we’re not culturally ready yet, or maybe I’m not ready yet. Pair programming does take place, but not on a formal basis and maybe 10-20% of the time on projects. Generally someone runs into a hard problem and calls another developer over once they break their frustration threshold. We do also have those horrible corner facing cubicle desks that pretty much preclude easy pairing.