ALM Practices Part 2: Peer Reviews

What is it?

A formal review of all code and artifacts related to a requirement or task by another person than the original developer. Rework because of review comments must be revalidated afterwards.

Why would you do that?

• Because the average developer
  • introduces 10-50 bugs per 1000 lines of code (at least, according to Moore’s Law)
  • is not always aware of the potential pitfalls of particular code constructs (hence coding guidelines)
  • is not always up to date with all available constructs in a new C# version
  • does not have full awareness of all the out-of-box solutions offered by the .NET Framework, open-source sites or commercial vendors
  • often does not realize that an ingenious and elegant solution may be too complex to understand by others.
  • sometimes loses sight of the overall solution and the initial goal when working on a some piece of code too long.
• Because it helps improve the awareness of, and the understanding behind your company’s coding guidelines.

Notice that the amount of necessary review work can be reduced by introducing the Pair Programming practice.

What’s the bare minimum you need to do?

• Find issues in the interpretation of the functional requirements.
• To verify compliance against your coding guidelines or standards.
• If you don’t have one, consider the Aviva Solutions C# 3.0 Coding Guidelines. Notice that these are an extension to Visual Studio’s Code Analysis, so the latter is still worthwhile.
• To find any loose ends such as TODOs or incomplete code. Resharper 4.5 includes a nice tool window that quickly shows an overview of all TODOs in your current solution. Resharper 5.0 even includes solution-wide analysis that finds dead code.
• To ensure that all code is easy to read and understand. For instance, all members and types have a comprehensive name and non-trivial code is annotated with code comments.
• To ensure that each and every check-in improves the code base, rather than introducing software rot.

What’s the usual thing to do?

• To ensure that all naming of code elements matches those of the functional design, preferably using Domain Driven Design’s Ubiquitous Language.
• To check for the usual pitfalls such as God classes or methods that are way too long.
• To find any redundant code or code that is unnecessarily complex and requires refactoring.
• To find any code constructs that may put other developers on the wrong foot and therefore jeopardize future maintenance.
• To find code constructs that can be replaced with a simple call to a previously developed generic class or .NET Framework construct.
• To stimulate the use of design principles such as S.O.L.I.D. or the design patterns defined by the likes of Martin Fowler and Eric Gamma so that the overall code quality increases.

How do you do that?

• It is important to make sure that all files related to a particular piece of functionality can be easily found. Use a Check-in Policy to associate each and every change with a corresponding work item such as a Scenario, Task or Bug. Since a major change may involve multiple check-ins, especially in a larger team where multiple developers work in the same areas of your code base, Change Sets provide insufficient traceability.
• Use a logging form or something similar to review every piece of code related to a particular work item and store it on the project’s team site.
• Alternatively, copy and paste the code into Microsoft Word, and use its reviewing features to annotate the code with comments and improvements.
• Or even better, install TeamReview and its corresponding Review Response work item and add review comments directly from inside Visual Studio’s code editor, while keeping them together under the associated work item. Also check-out this poster illustrating the underlying review workflow.