Several months ago, I got back on the code review bandwagon and jotted down some thoughts. As I worked with other teams doing “code reviews”, the lack of structure (from my perspective) got me energized to go back down this road. We have been playing with the Crucible Code Review Tool for a couple of weeks now, but I’m not completely sold on what I’ve seen; I feel like I’ve missed something, but still think this is the right direction to be heading.
I first want to share a couple of links to some vendor papers that I think are worth noting, Effective Code Reviews (Atlassian.com), and Best Practices (SmartBear.com). I found two points that I wanted to highlight from these documents, both of which are about “preparation”. The following graphics were pulled from referenced links.
This first one really drives me crazy, but unfortunately, this appears to be the way that most teams perform their “code reviews”. First, the presenter/reader sets up a phone line for all of the participants to dial into, even if they all sit together in the same area! Next, the presenter sets up a “net meeting” and walks the team through the code; adding comments or making small fixes along the way. Maybe I have too limited of an attention span, but I don’t really commit to this type of code review, nor do many others from my observations. Inevitably, someone comes by with a question, or you get an interesting email or instant message; something to draw you away from what is being presented. To make matters worse, you have probably never even seen the code before and maybe don’t even understand the design. How much can you really contribute, other than noticing the most obvious issues? The above graph shows the range of code reviews, from a formality perspective. Not that the process has to be this rigid, but I think it is important to discuss the stages and roles of an inspection:
- An inspection follows a well-defined procedure that includes six stages: planning, overview, individual preparation, inspection meeting, rework,and follow-up. Certain participants have assigned roles: author, moderator, reader, and recorder.
Depending on the team, there is usually only one stage (meeting) and one role (reader). I personally feel it is much more valuable to explore the code and actually read the code in advance, committed to providing feedback; I think the reviewer needs to physically participate, whatever that means: print off the code or browse the code on line, actually understanding the code, be engaged!
My second point is about preparation. I generally don’t think that most developers are very effective at “thinking on their feet”. It is really not about intelligence, but more about communication. Teams are typically very diverse, both technically and it the way they consume (process) information. It can be very difficult to present code at a level which will be effective for everyone to understand and process. In my opinion, no preparation is equivalent to GIGO.
Previously, I thought it was more of the reviewers responsibility to make the process meaningful. I think the reviewers should (must) prepare before the meeting, just like they were studying for a test. Feedback must be returned before the “meeting”; otherwise, there is no need to schedule the meeting. The purpose of the meeting is to discuss comments and determine the best solution for highlighted issues; this approach also helping to get everyone on the same page. The amount of preparation done by the presenter/reader can vastly impact the review as well. If the reader simply “wings it”, it will be a generally unfocused and undirected presentation.
As illustrated by the graph to the right, the reader’s preparation has a dramatic effect on the code review process. The process of the author reviewing and re-thinking their implementation before submitting the code for review, provides another opportunity to discover problems before the inspection process actually starts, making the process even more efficient. How many times have you looked at your own code and said… “What was I thinking? This is not good at all…” So, why know incorporate a “self-review” into the process as well?
I will wrap this up, so you don’t get too bored, my next blog will be on our Crucible evaluation. I did find a short blog that had a nice picture of the “Crucible Workflow“. This picture is where I’m striving to be, from a process perspective: collaboratively and iterative and ultimately actionable.
