Code

How to hold a more effective code review

~ Andrew Stellman ~ 2 Comments

A lot of programmers feel like being asked to go to a code review is like being told by mom to eat our veggies. We’ll complain about it, and even if we do eventually swallow them we’re determined not to enjoy them.

It’s something I’ve seen over and over again: programmers groaning about having to go to a code review, usually because someone gets some big idea about making things better, and decides this is how you do it. There’s sometimes a little nervous joking at the beginning of the meeting about how nobody really wants to be there. And after it’s done, a lot of us get the distinct feeling that it was a waste of time.

The thing is, code reviews can be a really good thing. And not only that, they don’t have to be a chore. If you do them right, people on the team can start to appreciate them and even – heaven forbid – enjoy them.

So how do we make code reviews more palatable? We need to think about what motivates us as programmers. Programmers love to code. We love building things, and we love solving problems. But we hate anything that seems bureaucratic or tedious, and we definitely hate meetings. But most of all, we hate being in uncomfortable social situations that require us to confront people face-to-face. We’re not alone in that; most people hate situations like that.

I think that’s a pretty big part of why programmers intuitively dislike code reviews. It’s not that we’re afraid of putting our work out there for our peers to see. That’s actually something we look forward to: we love to show off code that we’ve worked so hard on, and we definitely appreciate the input from the people around us. But it’s almost never the person whose code is being reviewed who groans about it. No, it’s usually the people who are asked to attend the review. And I think I know why: it’s because we don’t like being asked to criticize the work of others, openly and without hesitation, for the good of the team. I think we naturally feel uncomfortable putting someone else in the position of having to openly confront their errors, because it’s so easy to imagine ourselves in that same position.

And that may be the secret to holding code reviews that people actually look forward to attending: make it about helping make the code better, not criticizing and finding its flaws. If you can come up with a way to avoid bitter arguments about tiny details and stubbornly-held opinions, and instead concentrate on helping someone improve his or her code, then people will stop thinking about code reviews as an uncomfortable meeting and start thing about them as a way to build better software.

That sounds like a tall order. How do you do it?

Well, you start out by thinking about one of the best things that can happen in a code review, something that I’ve seen many times. It usually happens about two thirds of the way through the review, about the time when the first signs of meeting fatigue are starting to set in. Someone points out another problem with the code, and a conversation starts up about an aspect of it that nobody really thought of. Uh-oh — it’s a bug, a particularly nasty one that. Then everyone kind of looks at each other with a weird mixture of relief and disgust, because we found a bug that a) definitely would have made it into production, and b) would have taken hours or days to track down and fix.

I’ve said this many times before: programmers are very practical people, especially when it comes to our own time. If something will save us time down the road, we’ll definitely do it. If you can show a programmer that a tool or technique (like a code review) saves more time than it costs, that’s a great way to get him or her to start thinking positively about it.

That doesn’t change the fact that a lot of people get nervous about code reviews, even people who have done them a bunch of times. So I spent a little time thinking of advice I’ve given about code reviews over the years. Some of this is pretty standard code review stuff, but I think it’s worth repeating because people have so many different views on how to do code reviews effectively. And I think that if you think about them, and get other people on your team thinking the same way, it could definitely help you hold effective code reviews.

So here’s some advice about holding better code reviews — you can think of them as code review tools (or even code review best practices, although I’m not a huge fan of that term) that can make your software better:

  • First things first: there are a lot of different ways to do a code review. Some people like to follow a very strict process. Personally, I like to keep them informal; the more it seems like an everyday conversation, the more work we actually get done.
  • It’s important to keep the meeting to under two hours — any more than that and meeting fatigue sets in. Most code review teams can handle between 200 and 400 lines of code in a two-hour review meeting. (Your mileage may vary.)
  • Don’t forget to distribute the code before the review, and make sure you give everyone enough time to actually look through it. Send around a PDF of the code (a2ps is a good way to make it readable, and it’s got stylesheets for almost every language). Also make sure that everyone also has access to the source, and that they know how to build and run it. Sometimes it’s a lot easier to prepare for a code review if you can actually debug your way through the code.
  • Make sure that everyone knows you appreciate their time. It’s easy to forget that, but it helps the team see the review as a useful tool and not just another boring meeting. Remember, you’re pulling half a dozen or more people into a room for two hours, plus preparation time — that’s the equivalent of taking a top developer off of all projects for two days. That’s also why it’s very important to choose a good block of code to review. Choose one that’s inherently risky: a difficult algorithm, code from a library that many other people depend on, an interface a lot of people will use, a particularly nasty bit of spaghetti code.
  • Code reviews and refactoring work really well together. Look for opportunities to extract methods, rename variables, replace literals with constants, etc.
  • Pay attention to OOP principles, especially encapsulation. Improving encapsulation is an easy and effective way to prevent bugs.
  • The code review isn’t just about bringing up issues — it should also be about giving some indication of how to resolve those issues. Ideally, the programmer whose code is being reviewed should be able to read through the results of the review and very quickly implement the fixes, because in the meeting we wrote down exactly what needs to be fixed and how to fix it. (We don’t actually have to write down lines of code, of course — just enough information so it’s clear what to do.) A good way to do this is to make the goal of the meeting to be to produce a log, or a list of fixes that need to be made to the code.
  • Instead of storing the log in a spreadsheet, Word document, or wiki page (I’ve done all three), try having the moderator put the results of the review directly into the code as comments (which includes an easily searchable unique string like ‘// %%TODO: CODE REVIEW 8/28/08%%’, so the programmer can find them all). The results of the review meeting can be e-mailed out a link to a diff report in the source repository. When the programmer goes back to update the code, he or she can alter the comments to make sure they make sense in context — but they can stay in the code because they’ll make more sense, and it’ll be clear why the code is the way it is if someone is maintaining it in the future.
  • A good way to focus the discussion is to guide the conversation away from arguments about coding in general, and towards what to write down in the log to resolve the current issue. Make a good effort to figure out how to resolve the issue: most can be resolved in the meeting. Any issue that can’t be resolved in a reasonable amount of time gets added to the log as an open issue.
  • I’ve always gotten a lot of mileage out of using a moderator. The moderator’s job is to keep track of the discussion, and keep the discussion on track. If people are getting off onto a tangent that won’t benefit the code, or if they’ve gotten into a disagreement where there are merits on both sides of the issue and it clearly won’t be resolved, the moderator should suggest that we log it as an open issue and move on. You can always follow up later and resolve the issue.
  • Some people get very strict about making sure that the moderator stays at arm’s length, and doesn’t get involved with the review. Personally, I think code reviews are hard enough without imposing arbitrary rules like that (because we’re laying someone’s code bare and dissecting it, which can be difficult for anyone who’s not used to it). We’re all adults here, and we can trust any of us not to “abuse” a moderator role. If the moderator has something to say, he or she should say it. If it’s easier, replace “moderator” with “note-taker” or something like that.
  • Don’t be pedantic, and try to avoid theoretical discussions. It’s really easy to get bogged down with a discussion that doesn’t go anywhere about whether this variable declaration should be here or there, whether this type of structure or that is slightly more efficient, if we could do something better in theory if we scrapped a large amount of code and rewrote it. If a discussion isn’t going to directly lead to a change, even if it’s an interesting topic, note it in the log as an open issue and move on. And definitely don’t point out spelling errors. A lot of grate programmers are lousy spelers.
  • Make sure variable names make sense, but don’t worry about naming conventions. Some people love underscores in front of parameters, some people hate them. I’m sure you can come up with at least three different “official” conventions for any programming language. There are few things less useful during a code review than an argument over whether to use PascalCase or camelCase.
  • One way people like to do reviews is to have people “read” the code – interpret it out loud. I’ve had some success with going around the table and having people take turns “reading” each chunk of code. If there’s a chunk that is difficult to “read”, it’s not clear enough and is a good candidate for refactoring.
  • Before you distribute the code to the team, run it through a static code analyzer (like FindBugs or FxCop) and fix issues that are found. There’s no need to waste discussion time on problems that a tool can catch and log for us.
  • I’ve had a lot of success with a kind of review called a “high impact inspection” (that’s a term that was developed at HP about fifteen years ago). Basically, it boils down to having everyone do the code review independently and e-mailing their issues to a moderator. The moderator puts the issues into one big list, sends them back out to everyone, and then the review meeting itself is focused on just going through those issues. Jenny and I developed a code review process similar to high impact inspections to let us hold inspections in teams outsourced to India (where time zone differences make it very difficult to regularly schedule two-hour meetings). We ran it a bunch of times, and it worked really well.

When Jenny and I were writing the section on code review techniques in our first book, Applied Software Project Management, we came up with a checklist of things that you should look for during a code review. That should give you a good starting point for coming up with a good review.

Good luck with your code reviews! If you end up with a good code review success (or failure!) story, I’d love to hear about it.

Code reviews, user stories and documentation

~ Andrew Stellman ~ Leave a comment

I had the pleasure of spending some time talking about project management and software development with James Grenning, who was in town to do training for Object Mentor. (You can read his articles here.) We spoke about a wide range of topics, and I definitely learned a few things about code reviews, user stories and documentation.

James had some very interesting ideas. We spent some time talking about code reviews. He pointed out that when selecting code to review, it makes sense to concentrate on code that links objects together, rather than internal behavior of an object, since that’s what unit tests are for. That made a lot of sense to me — the unit tests Lose Weight Exercise the objects themselves, which means the risk of defects shifts more to the integration of objects. Targeting the reviews at the integration code is much more likely to catch exactly the sort of defects that unit tests would miss. (Note to James: I hope we get to see an article or blog entry giving us some details on this!)

The most interesting thing we talked about revolved around requirements gathering and XP user stories. I’ve long been of the opinion that while user stories can make good requirements gathering tools, they seem too temporary. They’re written on 4×6 index cards, and they never seem to be used again once the system is built. James pointed out that this is the point of user stories — they’re made permanent once they’re turned into acceptance tests. And then he said something very interesting: he pointed out that this eliminates the need to store redundant information about the project.

I think this is where James and I may have reached a disagreement. I think there’s a lot of value in having what might seem to be “redundant” information in different project documents. This is definitely counterintuitive for many programmers, since we’re trained to only store information once, and reference it rather than duplicate it. I think it’s very valuable to have, say, a rationale in a use case replicate some of the information that might be in the vision and scope document, or have a functional requirement mirror certain steps in a use case. Often, it’s useful to repeat a piece of information in order to make a requirement or use case more readable. And most of the time, that information isn’t necessarily redundant. A requirement may go beyond what’s written in the use case, adding information about how the software should function in order to implement it. When this requirement is reviewed, that extra information can provide a valuable quality check — if one of the team members has a misunderstanding in that area, it will make it likely that the two douments won’t reconcile with each other.

I think the basic principle at work here is that documentation isn’t created for its own sake. In some cases, like with user stories, the documentation is only an intermediate work product that leads to code and acceptance tests. Once those things are created, there’s no need to keep them around. On the other hand, project documentation like a vision and scope document or a use case has a different purpose: to make sure that everyone on the project team has the same understanding of what the project is supposed to do or how the software is supposed to behave. And, as we all know, while it’s more work to keep this sort of documentation up to date (through reviews, inspections and change control), this gives the team a lot of opprtunities to find and repair defects before they make it into code.

A few myths about code reviews

~ Andrew Stellman ~ Leave a comment

Have you ever noticed how hard it is to get people to do code reviews? It seems like developers and project managers alike are allergic to them. I’ve noticed that when I talk to people about code reviews, the same few objections come up over and over again. I think it’s worth looking at those objections, and why they don’t really make sense when you think about them.

Myth: Code reviews are a waste of programmers’ time.

For some reason, many programmers and project managers will shoot down code reviews, claiming that they’re a waste of time. In my mind, if a practice is a waste of time, it means that practice doesn’t save the project more time than it it costs to perform. And in my experience, this just isn’t true when it comes to code reviews. When I hold code reviews, I often find that the team ends up with a sense of relief because we caught a bug which would have been far more work to track down later on.

Typically, a code review with four people will requie about ten person-hours: each person spends about half an hour preparing, and the meeting lasts about two hours. In that time, we almost always identify a couple of defects which we probably wouldn’t have found otherwise. Everyone is keenly aware that had those defects been caught by the testers (or, even worse, the users), it would have taken a lot more than ten hours to track them down. It’s not uncommon for a code review to start out with a bunch of groans, only to end up with all smiles!

Of course, “data” isn’t the plural of “anecdote”. However, my individual experiences are borne out by many studies, which have found time and time again that code reviews (and, in fact, most accepted inspection practices) pay for themselves.

Myth: Code reviews aren’t effective because you can’t review all the code.

It’s true that code reviews cannot cover all — or even most — of the codebase for all but the smallest applications. Since code reviews can only catch defects in the code samples being reviewed, doesn’t this mean that they’ll only catch a tiny fraction of the defects?

But just because you can’t review all of the code in the project, that doesn’t mean code reviews aren’t worth doing! It turns out that if the right code samples are selected for review, then code reviews can be very effective. In most projects, a large number of defects tend to be concentrated in relatively small pockets of code. If the code is selected well, then the review will catch the most costly defects.

Note: You can use this code review checklist to help select the right code sample.

Myth: Given a chance, programmers will endlessly “tinker” with the code.

This seems to be one of those ideas that only managers come up with. I’ve never met any experienced programmer who would think of using a code review to “tinker” with code. It turns out that a code review is a pretty poor forum for some sort of mindless code “beautification”. Reviewers are not engaging in a top-down redesign of the code — they only bring up problems which they believe are defects. What’s more, the collaborative aspect of the code review meeting tends to discourage any changes that are only made for the sake of making changes. (It turns out that software developers are a pretty pragmatic bunch, and they have a low tolerance for useless behavior.)

Still, it’s good for the meeting moderator to be aware of this myth. That way, he can cut short any discussion which he thinks is leading down that path.
Myth: Programmers aren’t paid to sit around and talk, they’re paid to code.

There’s a great story in Peopleware by Tom DeMarco and Timothy Lister (which is a fantastic book that everyone involved in project mangaement should read). A programmer is sitting at his desk with his feet propped up, staring into space. His manager comes by and demands to know what he’s doing. The programmer says that he’s thinking. The boss asks, “Can’t you do that at home?”

Software development — in fact, any engineering activity — requires a lot of thought, and a lot of discussion. It’s rare for programmers to be given the time that they need to discuss the difficult problems that they routinely encounter. A code review is a great opportunity for a project manager to encourage that sort of discussion.