NYCPHP Meetup

[David Krings]

On 12/29/2009 5:30 PM, Peter Becker wrote:
> Looking to get some views (and best practices) on code reviews.

At my current work the devs do code reviews once a mod, project, or milestone 
is completed. The code review takes place before the peer review (show and 
tell to QA and TW) and other devs give input on best coding practices, 
violations of the development style guide, and general quality remarks. A 
first rough check happens during the tech spec review, but at that point only 
the approach is validated because no code exists at that time.
A lot of the review work can be covered by establishing a well-written, 
detailed style guide that everyone has to follow without exception. If you 
don't have a style guide, it may be a good time to create one and phase it in 
(there will be changes to the guide at the beginning). Adherence to the guide 
is supposed to make code be understandable to others in the team (or the rest 
of the world in case of OSS), follow common best practices, and include naming 
conventions as well as explicit rules on how to craft error messages and 
design the UI. Creating a style guide is a painful process, but one that is 
worth the effort especially when the team is likely to grow or has a high 
fluctuation of members (for whatever good or bad reason). It will also help 
when you contract out work. That may not be an issue right now, but when it 
comes up you are at least prepared.
Depending on how technical your QA staff is you may want to include them in 
the code review. While they may not be in a position to point out programming 
flaws by looking at the code they definitely will get a better understanding 
of workflow and learn why things are the way they are. QA should take a 
passive role here, their active role is in the peer review where they can make 
the developer crash his/her own code in front of an audience.

The mod/project/milestone based code review works fine for our developers 
because it focuses on one set of functionality and isn't all over the place as 
would be with reviews that take place on a recurring schedule. Also, all 
developers should properly prepare for code review and not just show up and 
give some cheap comments. The other devs should have looked at the code before 
and made notes. That makes the code reviews better and shorter because 
everyone is on the same page and knows what is going on. That includes 
knowledge of the functional requirements as well as the technical specs as 
well. If a dev sees the code to review for the first time during the meeting 
not much can be expected in regards to feedback. The code review meeting isn't 
really where the review should take place, but where the results of the 
individual reviews are discussed and action items are established. If the 
review went bad, schedule a second one before going into the peer review. At 
the time of the code review the developer should consider his code to be 
release ready, that means unit tests are completed and the whole thing works 
reliably. It is not enough to make sure that it compiles and maybe works for 
the intended use. QA will find the flaws anyway, so the devs may just fix it 
now rather than make more work for everyone. If that takes a day or two more 
I'd consider that time well spent.

David