Freigeben über

Motley says: "I just get picked on when my code is reviewed"

  • Artikel

Summary

 

Motley:  I'm not sold on this inspection thing. There were too many problems during our session. Plus, everyone just picks on the author and they hate having their code reviewed. Back to informal peer reviews for me.

 

Maven: Follow inspection best practices: inspect limited amounts, don't inspect everything, use a facilitator, be hardcore, use a checklist, focus on the work not the people.

 

______________________________

 

[Context: Motley has just finished his first code inspection with the team and is explaining to Maven how it went]

 

Motley: Well, we did a code inspection to try out the process we talked about. I gave the team an overview of the process using the diagram you gave me. We picked a bunch of code to look at from multiple owners, everyone prepared and we did the meeting. As I suspected, it really wasn't much better than an informal peer review. I'm not sold on this inspection thing, and I'm not sure I would do it again.

 

Maven: Hold on! Before you make that conclusion, let's get to the root cause as to why the inspection was not as effective as it could have been. First of all, how much code did you pick and how long did people spend preparing?

 

Motley: We inspected about 1500 lines of C# code. I would say the average preparation time was about 45 minutes, and the meeting took about 45 minutes as well.

 

Maven: There's problem number one. Every study on inspections indicates that less code in one session is better. In terms of absolute numbers, the guideline for preparation is somewhere between 200 and 500 lines of code (LOC) per hour . Generally, developers cannot maintain the "zone" of code reading and issue finding for more than 1.5 to 2 hours, so you should inspect approximately 400 to 1000 lines in one two hour session.

 

Motley: Are you serious?!? How would a company like Microsoft use this technique on all 50 million LOC in Windows Vista?!?

 

Maven: They wouldn't. You don't want to inspect every LOC that the team writes. Pull out inspections in the following situations:

  • The first few LOC for each developer in a development cycle. Having the team review the first few LOC that each person produces helps nail some problems right at the start. If a junior developer does not understand how to properly throw and catch exceptions, for example, they are going to make the same mistakes throughout their development. Having a team-based inspection at the start gets them on the right path from the beginning.
  • Critical code. Code that sits on a network or other trust boundary (e.g. an ISAPI filter running in the IIS web server) is a great candidate for inspections. The reason is that the consequences of someone hacking that code can be severe, particularly if the code is running as SYSTEM.
  • Complex code. Complex algorithms and interactions (e.g. concurrent sections of a program) should be looked at closely by the team. In many cases, tools do not do a great job of finding issues automatically, and inspection is the best way to flesh them out.

 

The key message is that you should wisely choose code to inspect, and not count on inspecting all of it. You said average preparation time was 45 minutes, but was everyone prepared for the meeting?

 

Motley: Well, a couple people who shall remain nameless didn't spend much time on it at all. Coincidentally, they didn't report many bugs either.

 

Maven: If some people come unprepared, you have a few options:

  • Ask them to simply be an observer in the meeting. Since they will not have much to contribute anyway, you want to minimize their distractions.
  • If it happens multiple times, kick them out of the meeting with a clear message that they are not helping improve product quality, and that practice is not acceptable.
  • If people say they prepared, but you are skeptical, play a little game. In the code that is handed out for the inspection, inject a couple of bugs. Don't make them too difficult, but they should not be too obvious either. Good candidates are off-by-one errors and reversed Boolean values. Tell people how many bugs you put in, and challenge them to find those bugs.

 

What about the meeting itself? How did that go?

 

Motley: Not so good. We were all over the place. We had many tangential conversations where we tried to solve the problem we discovered. We also had trouble referring back to the lines of code that were troublesome and everyone had different copies of the code.

 

Maven: There's a few things we can do to help that situation.

  • First, all good meetings - and all good inspections - have a facilitator. With inspections this role is often called the "moderator." This person guides the meeting, collects the data and issues that are found, and keeps the meeting running efficiently. They have free reign to cut someone off that goes down a tangent.
  • Second, avoid discussing solutions. The inspection is all about finding problems. If you want to talk about solutions to specific bugs, put it on a list on the whiteboard (often called a "parking lot") and anyone that is interested can stay after the main collection meeting is over.
  • Third, it is important that everyone inspect the same copy of code. One way to do this is have the moderator print out the code for everyone. Inspecting code on paper removes you from the distractions of the computer. In addition, you ensure everyone has an identical copy, and by including line numbers, you have referral points for individual LOC.

 

If you follow those tips, you should have a better inspection. What kinds of problems did you find this time around?

 

Motley: Not much. We found a few cursory bugs, but nothing major. My gut tells me that there are more bugs lurking, but I cannot say for sure. The problem was that no one really knew exactly what to look for.

 

Maven: When preparing, you really need to focus. Be as thorough as possible when finding issues. Everyone should be familiar with the problem, team coding standards, and team development processes. You want to point out every little thing you find - be hard core. Don't just focus on pure logic errors. Anything that, as a quality-oriented developer, you would like to see fixed, point it out. By following this practice, we lead ourselves towards better quality products.

 

In addition, do an analysis of where your team makes frequent mistakes. Those items should be put on a checklist, and everyone should leverage that checklist while preparing for the inspection.

 

Motley: Everyone is familiar with the .NET Design Guidelines, but we need to be a little more hard core. It would also help if we ensure tools like FxCop are run prior to the inspection too. I assume that's why you have been using this word "issue" instead of "bug" over the past few minutes? You want devs to think more widely about the mistakes they look for? Comments that do not match the code count?

 

Maven: You are a very bright guy, Mot. That's exactly why I use the word "issue". One last thing: you mentioned you used code from several authors. Try and restrict the code you look at to one person to prevent context switching. Was that person happy to have their code inspected?

 

Motley: Quite the contrary! He felt he was picked on the whole time and even told me afterwards he wants to avoid this whole process in the future. I told him to quit being a baby and that it's his job.

 

Maven: Whoa. Probably not the best response. Anyway, authors should want to have their code inspected. It's a great learning experience. Inspectors can encourage this by avoiding a threatening environment. Direct comments at the work item and not personally at the author. Also, instead of making statements, ask questions. For example, "I'm not sure, but is there a memory leak on line 34?" is better than "There's a leak on line 34. What were you thinking? Did you not run the tools? Wise up dude!" Asking questions is less confrontational and who knows, you may be incorrect about your assertion. Let the author come to the conclusion herself. It's a better learning experience that way anyway.

 

Motley: That's a lot of stuff to remember.

 

Maven: It's not that bad. Just remember a few key points:

  • Inspecting less is better
  • Be wise about what you inspect
  • Use a facilitator in the meeting
  • Be hardcore about the issues you identify, and use a checklist
  • Remove the people from the problem (i.e. avoid threats, ask questions, focus on the work)

 

Motley: Ugh. Back to the drawing board. I can think of some nasty multi-threaded code that perhaps we should inspect next. I'll try this stuff out again if my team doesn't hate me and get back to you.

 

______________________________

 

Maven's Pointer:  According to Steve McConnell in Code Complete 2nd Edition, inspections are one of the most effective practices for finding defects early in the software lifecycle. In fact, studies have shown that inspections find 45% to 75% of all defects, with other practices like unit testing and system testing falling below these values. Your experience may vary, but selectively using inspections on requirements specifications, designs, code, test plans and other artifacts will save you time in the long run.

 

Maven's Resources: 

Comments

  • Anonymous
    November 07, 2007
    There's an old book by Gerry Weinberg (and someone else) on Inspections and Walkthroughs.  I recall that it dealt very clearly with the social issues of inspection and fostering a safe space where developers would be encouraged to desire inspections and the meetings are productive. [Found it: It is in McConnell's list on  p.76 of Rapid Development.]

  • Anonymous
    November 07, 2007
    Thanks for the pointer Dennis! Rapid Development is still one of the best software engineering books out there, even though it was written in the last decade. A paper I forgot to include is this one: "Humanizing Peer Reviews" by Karl Wiegers (http://www.processimpact.com/articles/humanizing_reviews.html)