Code Reviews – the way it should be?
This past Monday and Tuesday, I attended all-day training sessions led by David Hussman. He’s a big Agile guy, but more than Agile he kind of picks and chooses the parts he believes work – i.e. he’s huge on TDD and pair programming and continuous integration. Pointedly, he said if you don’t have the chops and you can’t figure out the hard problems then it doesn’t matter what process you follow, which I found refreshingly honest. Agile is not the panacea that a lot of coaches make it out to be.
The format of the class was very interesting. He managed to touch on all the topics he had to present in his slides, but we didn’t actually go through all the slides. About 30 minutes into the course, he decided that the best way to go would be to shut off the projector and tailor the sessions to our particular project work. Woo!
We started off by listing what we hoped to get out of the class, and then we all made stickies of the things on our project we thought could stand improvement. Once we had tons of stickies (and there were tons), we collaborated to group them into like topics (with much frantic scrambling). The areas that came out were things like: Teams, Culture, Accountability, Testing, and CI setup.
We voted with dots on the topics that were most important to each of us, and the ones with the most votes got discussed first. For each topic, he put up three columns: Could Change, Might Change, Won’t Change. I was pleased that, while we certainly couldn’t fix every problem, there were MANY things identified which we could change. We put our action items up on the sheets and at the end of the day we assigned names to each for follow-up.
One topic we discussed was the subject of my previous blog post (code review observers), and this was an area where he really challenged us. I bristled a little when he said, “You’re coming across very old school in your adherence to the idea of code reviews.” His point was that some of the best code he’s ever seen was code that was TDD’d and paired on. It didn’t need a formal review. He pointed out the value-added cost of code review can outweigh the benefits. Catching bugs in review is a very reactive (rather than proactive) approach.
At SEP, many of us come from FDA or FAA backgrounds where the formal audit process makes code reviews mandated. I’ve heard it joked about that taking away code reviews would be like taking away our security blanket. When David commented on this, I had to think long and hard about whether or not I agreed with him, because reviews are very engrained into our culture at SEP. One of my favorite authors, founder of Stack Overflow Jeff Atwood, lists peer reviews as “the single biggest thing you can do to improve your code.” Granted, he said that in 2006. Prior to that, pioneer Steve McConnell of Code Complete touted the effectiveness of design and code reviews by showing studies that they are actually more effective at finding bugs than unit or integration tests.
So do you get the same benefits when you pair real-time? I’m inclined to say that when it is done right – yes, absolutely. I’ve been part of a pair where we cranked out some amazing code test-first and both walked away feeling exhausted. It was awesome. I know that code is much more solid than it would have been had I written it solo, but I wonder, is it more solid than what I would get from a code review? Possibly. What about the cost – is the overall time investment (time == money) smaller for the pair? Not sure. These are ideas that need further pursuit.
In a perfect world, every developer would be paired up and TDD’ing and walking away exhausted. In the world of our project, a recent anonymous survey showed that TDD and pairing aren’t being used consistently. With 20+ teams, and only 3 of them at SEP, getting everyone moving in the same direction is going to be challenging no matter what. I am sold on pairing and TDD – I don’t believe anyone on our SEP teams would disagree. I appreciate that David Hussman made me think critically about something I took for granted as “the way it should be done”. I need to continue to question stuff like this and to learn new techniques if I’m going to grow as a developer. But, for now, I am not quite ready to give up my security blanket.