• Witch hunt reviews - Many reviews degrade quickly into attack and defend mode. This often occurs because the developer who wrote the code feels attacked and threatened when reviewers make direct and opinionated statements about the code. Nothing could be less productive.
  
  
• Curly brace reviews - Some reviews emphasize formatting and comments instead of more serious problems. Is placement of curly braces and misspelled comments really that important? Curly brace reviews are feeding ground for the anal retentive, and provide no real value.
  
  
• Blind reviews - Often times, reviewers walk into the review meeting having never laid eyes on the code they are about to review.  Most of the review time is spent trying to figure out what the code does. Spending time in the review meeting attempting to understand the code instead of reviewing it for more serious ailments is a waste of time.
  
  
• Exclusionary reviews - Many times, the code provided for the review is only a sampling of the code written. For example, unit tests might be excluded from the review. In an unhealthy review environment, providing impartial and incomplete code listings will leave the reviewers wondering how the code actually works.
  
  
• Tree killer review - If you can't baffle them by providing half of what they need to understand the code, then maybe overwhelming them by providing thousands of lines of code might work. Waiting until the codebase is incredibly large to host the first review is entirely ineffective. Not only is it to difficult to provide effective feedback on a large codebase, these reviews are often held late in the lifecycle and do not allow the developer to improve her code based on the feedback received. 
  
  
• Token review - It's not uncommon for management to dictate that reviews be held. In fact, that's the reason I held my first review. Fortunately, I decided that if I was required to host them, I was going to try to make them effective. Token reviews are typically held for political reasons. Management wants to ensure that all code is reviewed for auditing purposes. Unfortunately, developers realize very little value surrounding these reviews. Any problems found are not fixed unless they are absolutely critical. Since the primary motivation is an audit trail for management, the team has little motivation to improve the code.
  
  
• World review - I've been to some reviews that probably should have been held in an auditorium given the number of people in attendance. This can be incredibly intimidating for the developers whose code is being reviewed, and I'm not sure what value it provides to invite so many people. A few developers, up to five, should serve all the needs required of the review process. If more people want to provide input, there are better ways.

• Design quality  - Design metrics turn attention away from developer's opinion on what constitutes robust design, and toward metrics that provide objective feedback on the quality of the design. JDepend is an example of a tool in the Java space that offers important feedback on design quality.
  
  
• Code cleanliness - Unused imports, references to implementation types instead of interfaces, and empty catch blocks are all common coding mistakes that can easily be caught by static analyzers such as PMD.
  
  
• Code quality - Exceptionally large methods with complex conditionals are a significant maintenance burden, and are incredibly difficult to test well. A simple tool like JavaNCSS that provides a glimpse into the cyclomatic complexity of each method is incredibly valuable.
  
  
• Test coverage - Well-tested code is less likely to contain defects. Evaluating the percentage of code under test is an important quality that impacts how easily code can be refactored when necessary. Emma is an example of a tool that determines test coverage. A newer tool, called Crap4j, combines coverage metrics and cyclomatic complexity to help identify poorly written Java code.

Comments (0)

Write comment

Show/hide comment form