Code Review

[elainegrey]

This is a question for professional coders and managers of coders and anyone for whom the phrase "transitioning from waterfall to agile" is clear. You wanna point my friend your way? Feel free.

Background: five developers who have worked together for 10-25 years in a mainly waterfall environment, but where each essentially owned code and did all the adaptions and maintenance etc etc etc. Last code review was for stuff on the mainframe.

New project started in July where the team must conform to coding standards, more complex implementations and architecture, complicated project versioning and so on. We have a New Guy who is saavy to the modern stuff. Morale is mixed. Some folks are very frustrated because they're used to getting the requirements implemented and done (well! and working! and right!) but they're used to doing it their own way (and turn over is so low that the cost of this behavior has never been a management motivator to change it). Some are happy to be learning new (marketable) techniques.

Code review is a strongly encouraged part of the mandated standards.

I understand how code review is a Good Thing. "What's that?" "Oh, i'm decorating the method with the validation aspect so blah blah blah...." Folks can learn. There are so many automated tools that check the styles, review for possible bugs, evaluate whether there are enough unit tests that the code should be pretty scrubbed. this blog entry on code & complexity describes that after all that scrubbing "...the primary reason we formally review code is to subjectively comment on other possible ways to accomplish the same functionality, simplify its logic, or identify candidates for refactoring."

At this point, i don't know that we have a team that's ready to think about refactoring their code, partly because they are spending much of their time refactoring and adding onto inherited code. The goal for me as a manager is more that more folks gain confidence in the work they are doing. While a review is threatening (as i've ascertained by the revulsion some express) it may also help folks feel more confident about the work they're doing.

I'm trying to imagine the process of review in a way that is efficient (not having five folks and a manager and an analyst all go over the code together) and effective.

Right now i am thinking of having my egoless senior coder (he's confident, he takes criticism well) review his code with my most junior coder (so she can learn) and New Guy (because he's our expert).

Should i be there?

Should this be informal: i suggest to A to gather B&C together when his code is scrubbed right before he's ready to commit?

Can this be done with a webex? Because we have lots of working at home folks.

Should there just be a regularly scheduled code review block twice a week and they who are about to commit review each other's code together?

Do you have happy code review stories?

Comments

[arlie]

weofodthignen sent me over here because she knows I've been working in this industry for 30 years, and figured I might have useful things to say. Let me start with the bad news - most of the places I have worked, the managers liked code review better than the developers did, and tended to treat it as a panacea, suitable for solving problems it couldn't handle as well as problems it can handle very well. It's important to recognize the limitations of code review, as well as its virtues. The same thing tends to apply to standards. That said, I like both code review and standards, and notice definite problems when I move to an environment with too little of them.

I believe what is needed is a variety of code review styles, using methods appropriate to the work being reviewed. This ranges from an email "Joe, I just checked in a trivial fix - could you take a look at change list 100379 when you get a chance" to gathering a large team into a scheduled face to face meeting for a formal inspection/walk through.

Just about everywhere I have worked, managers have not been invited to code review meetings - sometimes that's been formalized, as folks feared that errors found in their code would be remembered at performance review time. It's different for the kind of team lead who programs and does a bit of managing on the side... I remember one instance where such a guy started asking me to review his (easier) changes shortly after I was hired - and was rather surprised when I found 2 bugs in the first one. (His response: "the process worked.")

Non-face-to-face methods have worked well for me. One employer had a web based application for code review comments. The interface was annoying and ponderous, until one of the developers who had to use it rewrote it, but given that it was great - supplemented by face-to-face and/or phone or email conversations when things got complex. (That was also the job where me and the guy sitting next to me decided we liked semi-extreme programming methods, and happily worked that way a lot of the time - not part of the official methodology, but we were working on a lot of hairy, fundamental code, and saved ourselves a lot of time using two sets of eyes _before_ testing etc.)

I believe that in general, reviews are done at the wrong time. By the time someone has fully tested and personally reviewed their code, there are very few bugs left - only questions of style. They are very attached to it, and some deadline is fast approaching - a suggestion that they change things will not be received very well, unless it comes complete with an honest-to-god bug. Moreover, when they do make suggested changes, they tend not to test as thoroughly; I've seen a huge number of bugs introduced as the result of code review feedback. It gets worse if there's a need to merge one's changes with someone else's work on the same files - delays caused by review lead to bigger merges, and a hurried programmer doing the merge. Managers promptly mandate that all the testing and review be redone after any change, including merges. Productivity and morale both fall through the floor, and programmers promptly start cutting those corners - colluding on rubber-stamp reviews if necessary.

When I'm doing something I consider risky, I bring reviewers in early, and informally. "Heh Joe, have you seen the bug in xxx? It looks like yyyy is the cause, and I'm thinking of doing zzzz to fix it. What do you think?" "Sally, could you take a look at this code. There's something wrong in here, and damned if I can see it". I'll sometimes send code to official review while I'm still unit testing it, noting how far I've gotten.

At the moment, I'm being a reviewer for two large changes that have turned into collaborative projects. In one case, the developer thought the code was ready for submittal. Part of his work was outside his usual area, he had not gotten design review/approval from people that understood the problem space, and he had created what his reviewers have been calling a prototype. It worked - but would not scale to the size he actually needed - as well as being equipped with security flaws and breaking a large piece of functionality that had probably been implemented after this work had been begun. It took some work to convince him (and his manager) that the folks asked for last minute review weren't just a bunch of nay sayers, but at this point he and I are working together to fix those issues, plus a couple more that surfaced later. My next inter-personal hurdle may be convincing a third engineer that it's not sufficient for the developer and I to just review each other's code here. (It's a big block of code, under-documented, and very ground-breaking.) The guy I want as another reviewer is very senior and very busy - but he's the one who spotted the design flaw that made scaling impossible, and really knows his stuff.

The other thing I've noted about reviews is that they tend to get forgotten when the schedules are written. A regularly scheduled code review block might be a very good idea. It may not always get used - some weeks there may be nothing ready - and other weeks it won't be enough. But it might help a lot. The other thing to schedule is rework and retest time. Schedule pressure is a great incentive for cutting corners there, which you don't want. (That said, if the developers agree that the rework etc. is trivial, believe them. Sometimes it is.)

One more point - it should go without saying, especially given modern technology, but I'll say it anyway, given past experience. If using face-to-face meetings in conference rooms it's important for reviewers to prepare in advance, but it's also important for the code to be available at the meeting. With modern tech., the answer is laptops and a projector in the conference room. Make sure the blasted thing is working, and hasn't been "borrowed." Also make sure that folks know how to use it. Moreover, be sure that the conference room has functional ethernet jacks and/or wireless access. (This should be obvious - but I've had one recent employer which had plenty of useless conference rooms, with neither projector nor functional network access.)