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 _______________________________________________ New York PHP Users Group Community Talk Mailing List http://lists.nyphp.org/mailman/listinfo/talk http://www.nyphp.org/Show-Participation
