Peer Reviews

[Peter B]

I would be interested to know other developers views on Peer Reviews. How you conduct them, whether you formally record the results, how useful you find them (from both points of view) and what do you look for e.g. defects, security, readability, standards, effectivness, relevance, maintainability, etc.

Thanks

[allistar]

Peer review - one of my favourite topics

I'm am advocate at one of my clients for peer review to be done on all changes to their Jade schemas. They (we) do peer review on 80% of all changes.

What we check in peer review is:
That code syntax follows standards, that obvious bugs are found and removed, that obviously inefficient structures or code is dealt with, that objects (persistent and transient) aren't leaked, that there are inverses defined where required, that properties/methods are protected where appropriate.... (the list goes on).

The reason I advocate this is:
It's more efficient to fix bugs during the development phase than at the UAT phase. It ensures that code and structures follow a standard format so new and existing developers see a familiar style throughout the codebase - this in turn makes maintenance easier. And importantly (this one is key): there are often bugs in the code that normal UAT will not (and can not) detect - only a developer can. Examples are leaking objects, assertions in the right place, inefficient data structures or code etc. Also, developers get to share their skills and learn from other more experienced developers.

The most common reason from management of not doing peer review is the additional resource it uses. My opinion is that in the grand scheme of things doing a proper peer review actually uses less resources. It's a lot cheaper to fix bugs early on in the development lifecycle than having a user find the bug.

We don't formally record the results of peer review - the original developer would address any items raised in the review and then those changes would be reviewed again. Lather, rinse, repeat until the review is clean. The typical lifecycle is: spec, code, review, uat, release. Reviews are often done on specifications and the UAT process too.

Another approach that we have taken is to automate the review of certain easy to check things. This alleviates the effort required on the part of the reviewer and lets them concentrate on the tricky parts of review. Examples of easy to automate review is: do Boolean variable/property names start with a verb (is, does, will, should etc)? Is our naming convention adhered to? Are labels on forms capitalised correctly? Are methods/properties protected when they should be? If an transient object is created in a method, is it deleted in the epilog of the same method? Do labels on forms line up nicely? Do references start with "my" and collections start with "all"? Etc. There are many more things we automatically check. This is done using a schema we developed which has a Jade parser coded in Jade - you throw it a schema entity (class, schema, method) and then you can run tests on those entities. This is integrated into other schema(s) by way of an exported package. The parser produces an AST (abstract syntax tree) of the Jade code which allows us to easily do things like check if a iterators leaked in a method. The problem we have with this tool is that it is too fussy - it picks up a lot of trivial stuff that a person would normally ignore.

Anyway, I did warn you it was my pet subject. In my opinion developer peer-review should be done on everything.

[rbarr]

Peer reviews offer most benefit to a project when the reviewer has more experience than the programmer. This may be business domain, or engineering experience, or both.

They are more likely to see the woods for the trees, and spot design or code structural issues. This is where the benefits really kick in.

So reviews fit in well with mentoring programs.