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.)
![[personal profile]](https://www.dreamwidth.org/img/silk/identity/user.png){kind=small}
timeasmymeasure
no subject
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.)