The Code Review Potluck


(4 comments)
October 17th 2008

Sure, deep down I know that code reviews are good for me - data and conventional wisdom show that they can reduce defects, improve code quality, eliminate redundancy, and spread key project knowledge across the team. But then again, I know that dieting, writing my congressman, and visiting grandma are good for me too, and sadly I don't really do those things either.

The problem is, like with most things that are good for me, the typical Fagan-style code review takes a lot of time and discipline - two things I have a shortage of. Instead, what I want is a code review process that's more light-weight - something that has many of the benefits, but fewer of the costs. Code Review 2.0, if you will! (Ok, maybe not with that name, but definitely something different.) Specifically, I need something that's less rigorous than the standard code review, more breadth-oriented than depth-oriented (maximizing code coverage), and a bit less tedious for the participants.

That's what I was looking for on a recent project at least, and I came up with something that worked for us. I call it the Code Review Potluck. Here are the rules:

Step 1 - Prepare: Wouldn't a Potluck dinner suck if everyone brought over the same recipe of Meatloaf? Well, it's the same for code reviews - they're way more boring and less effective if each developer makes the same comments on the same code. For that reason, in a Code Review Potluck each developer focuses on where ever they are in the code that week, and prepares a list of 4 questions, 4 code quality concerns, and 4 kudos to share with the group. To be clear, here's what I mean. "Questions" are really problems, but for the purpose of tact, we phrase these as earnest queries. "Did you mean to put 3000 lines into that method named doStuff()? Really?". Next, "code quality concerns" are braces and spaces type stuff. Limiting these to 4 is key - no one wants to sit in a meeting and hear Ted list off every instance of a method I forgot to comment. Trust me. I got the point. And lastly, "kudos" are crucial. Just like in any friendship/partnership/marriage, the more good things you say about me, the more I'm willing to listen. It's just human nature. Try to find something nice about what I wrote ("wow, your use of the semi-colon is very smart!"), and I'll better remember your abstruse Javadoc pecadillos.

Step 2 - Enjoy: At the Code Review Potluck, each developer takes a turn sharing their list of goodies. For every item, we all look at the code (i.e. on projector or laptop) and discuss for a few minutes what's going on. "Do you agree that aggregation would be better than inheritance here? What do you think about this naming convention?". And so on. It's important, as Mom told us, not to play with our food - time is of the essence. Make your points, discuss a bit, and move on. In general, an hour for the Code Review Potluck is plenty. Less is more.

Step 3 - Digest: After the Potluck's over, one person gets the honor (read: burden!) of posting the questions/concerns/kudos and thier respective resolutions to the Wiki (you do have a Wiki, right? If not, email will work). Each developer then strives to make changes to their code later in the week, but let's be honest, there are no real guarantees (this a light-weight process, remember?). The real value of the Code Review Potluck is the digestion of the conversation itself. After spending an hour talking about code you'll hopefully have picked up some new tips, tricks, and practices that you can apply the next time you spend some quality time with your IDE.

Like all things "good for you", it's best if you make Code Review Potlucks a habit. Once a week over lunch was reasonable for our team. The more you do it, the easier it'll be.

That's about it! There's of course so much more that could be pontificated on regarding code review practices, benefits, costs, etc - but why make things hard? The Code Review Potluck can be a simple, helpful, and fun mechanism for improving quality on your projects. Good luck!


Resources

I'm an "old" programmer who has been blogging for almost 20 years now. In 2017, I started Highline Solutions, a consulting company that helps with software architecture and full-stack development. I have two degrees from Carnegie Mellon University, one practical (Information and Decision Systems) and one not so much (Philosophy - thesis here). Pittsburgh, PA is my home where I live with my wife and 3 energetic boys.
I recently released a web app called TechRez, a "better resume for tech". The idea is that instead of sending out the same-old static PDF resume that's jam packed with buzz words and spans multiple pages, you can create a TechRez, which is modern, visual, and interactive. Try it out for free!
Got a Comment?
Comments (4)
Jason Cohen
October 20, 2008
Nice article! I especially like the idea of having each person specialize in one topic. It's a more practical approach to the role-specialization that Fagan requires.

One suggestion for improvement -- there's no reason why humans should spend their time looking at code style (brace structure, token-naming) when static code analysis tools can do it automatically. Developer time is too valuable for that! And this is an unbiased suggestion: My company makes a tool for human code review (http://codecollab.com), not for static analysis!

For another suggestion to your "Resources" section, would you accept a selfish promotion of my book about lightweight code reviews: http://codereviewbook.com. Half the chapters are available on-line.
Ben Northrop
October 20, 2008
Thanks for the comments, Jason! Actually, a number of us at Summa recently read your book and liked it a lot. Very cool stuff. It's part of what inspired us to get more disciplined about applying code reviews on this recent project, to great benefit (in my opinion).

And, yup...I'm a big believer in the use of static code analysis tools - a great first-line of defense in the war against code slop. I actually created a report-engine wrapper around PMD to help show trends in code quality (PMDReports) and we're using that as well (although, admittedly, the tool's got a ways to go).

Anyway, thanks again for the comment! I will be sure to check out the tool.
Michael Chermside
July 22, 2009
Within my team, we have developed a code review process that seems fairly lightweight and emphasizes the *other* advantages of code review besides finding bugs.

Basically, before each person checks code in, they grab a few minutes from another developer on the team to read over the diffs. The person checking it in explains what's happening in the change; the other person makes comments.

There are several things I like about this process:
(1) It helps make sure that multiple people on the team can support any given piece of code. As a minimum, at least 2 people have seen every piece of code.
(2) It allows people to provide feedback at a particularly good time for actually making the change. The code is fresh in the original author's mind (they JUST finished writing it), and it hasn't been checked in yet, so there's not even a record of the original mistake. Things that get put off often never happen, and this doesn't happen with this sort of code review.
(3) It causes brief conversations about code style, design, etc. You don't get the same person every time (because they're not always available) and so you wind up having conversations with different people on the team. Usually both learn something small, whether it's a useful idiom, the existence of a handy library, or a potential bug pitfall. And the best ideas get shared again, so the whole team learns.
(4) The author of the code must explain it. It's astonishing how often they catch logic bugs just by trying to explain it. This is *far* more common than the reviewer catching bugs, probably because the author is the person MOST familiar with the code.

Anyhow, it has worked for us, and I thought I would mention it.
Michael Chermside
July 22, 2009
And I forgot to mention one other thing. Because we're reviewing just one commit at a time, it's a fairly small amount of material to review, which takes only small amounts of time.