Objective Code Review

Code review (peer review) is an established software development practice. Having another pair of eyes to validate an implementation is a proven quality control measure. It facilitates collaboration and knowledge sharing between team members. On the flip side, it creates an avenue where opinions and personalities collide which can hurt the team morale and hinder productivity.

This article establishes a formal approach to conducting code reviews in an objective methodical manner. It aims to take personal opinions, biases, and attitudes out of the equation.

The described code review process was established in 2011 during the early development phases of Magento 2. It arose out of frustration with subjectivity of the views and disagreements between opinionated architects/developers during mutual code reviews. Since then the process has matured and been successfully applied on projects of various sizes using diverse technologies.

Review Process

Actors:

• Author — developer who implemented a Changeset
• Reviewer — engineer qualified to conduct code reviews

Workflow:

1. Author ensures the Changeset is free from Code Defects
2. Author submits the Changeset for a code review
3. Reviewer validates the Changeset against the Defect Classification
4. Reviewer identifies Code Defects in defect comments
5. Reviewer posts recommendations in non-defect comments
6. Reviewer accepts the Changeset if no Code Defects have been identified
7. Reviewer rejects the Changeset if any Code Defects have been identified
8. Author fixes the identified Code Defects
9. Author considers non-defect comments

Roles

Author

Author takes full responsibility for his/her implementation to be defect-free. Reviewer is a safety net and not a substitute for personal quality control.

Author must be intimately familiar with the code review process to foresee its outcome. The code review workflow should not be started until there's a confidence in the implementation passing without any changes.

Reviewer

The Defect Classification plays the role of the quality arbiter, not Reviewers.

Reviewer must identify Code Defects and clearly articulate why changes fall under respective categories of the Defect Classification. It is helpful to outline an alternative defect-free implementation.

Reviewer can use non-defect comments to propose optional changes deemed to be beneficial. It's at the Author's sole discretion whether to act on such recommendations or ignore them. Reputation and communication skills of the Reviewer will be a factor here.

It's up to the team how to appoint Reviewers. Strong teams where members are on about the same level, can review after each other. Less experienced teams may benefit from external senior engineers conducting code reviews. Dedicated part-time reviewers will become a bottleneck and team members should grow into reviewing one another.

Code Defects

The Defect Classification represents the baseline code quality requirements imposed on all code changes. It's a collective team effort to refine it.

Once the team agrees to formalize a substandard coding practice as a Code Defect, it becomes a hard requirement to follow from that point on.

The Defect Classification:

• 1. Standards Violation
• 2. Logic Duplication
• 3. Unreliable Implementation
• 4. Technology Misuse
• 5. Performance Degradation
• 6. Security Vulnerability

1. Standards Violation

Established standards and adopted conventions must be obeyed at all times. The team documents an exhaustive list of the standards and can amend it.

Scope:

• Coding standards for each technology
  • Naming conventions
  • Typos, misspells, grammatical errors
• Automated testing standards
  • Types of tests: unit, integration, functional, performance
  • Code coverage

2. Logic Duplication

Copy-pasting any non-trivial code identically or with variations is discouraged.

Scope:

• Copied file
• Copied business rules
• Copied algorithm
• Copied expression: formula, regular expression, SQL, XPath, etc.

3. Unreliable Implementation

Unreliable code may produce an error or behave inadequately under specific conditions or on certain input data.

Scope:

• Backend
  • Unsafe data manipulation
  • Data type assumptions
  • Data structure assumptions
  • Unreliable error handling: suppressing exceptions
  • Dead code: unreachable statements, unused dependencies
  • Data corruption risk: unhandled race conditions
  • Infinite loops
  • Infinite recursion
  • Errors in logs
• Frontend
  • Incompatibility with supported browsers
  • Unsafe DOM access
  • Dependency on timing of events
  • Errors in developer console
• Database
  • Mishandling transactions: asymmetric begin & commit/rollback
  • Data integrity issues: missing foreign keys in RDBMS

4. Technology Misuse

Technologies, languages, and frameworks should be used to the full extent of their capabilities according to established best practices.

Scope:

• Project directory structure
• Ignoring built-in capabilities of a language/framework
• Alternative implementation of existing functionality
• Violation of dependency boundaries between application tiers

5. Performance Degradation

Performance impact of code changes should be avoided as much as possible.

Scope:

• Backend
  • Heavy operations in constructors
  • Loading unused data
  • Computing unused data
• Frontend
  • Loading unused assets: CSS, JavaScript, images, fonts
  • Executing no-op JavaScript
  • Excessive AJAX requests
  • High network bandwidth usage
• Database
  • Missing/unused indexes
  • Repetitive database queries
  • SELECT more records than necessary
  • CRUD multiple records one by one

6. Security Vulnerability

Potential security vulnerabilities must be avoided regardless of whether they're exploitable at the moment or not.

Scope:

• Infrastructure
  • Unnecessary public access
  • Unencrypted connection: HTTP, FTP
  • Compromised hashing algorithms: MD5, SHA1, etc.
  • Insufficient encryption strength: SSL, TLSv1, etc.
  • PCI DSS compliance violation: storing credit cards
  • OWASP Top 10 threats exposure
  • CORS misconfiguration
• Backend
  • Insufficient user input validation
  • Dangerous functions: shell, eval, file system functions
• Frontend
  • Rendering unescaped markup
  • Non-secure cookies accessible via JavaScript
• Database (can be considered misuse of a framework)
  • Raw SQL
  • Bypassing DBAL
  • Bypassing query builder
  • Not using prepared statements

---

The code review process is a template applicable to almost any tech stack, particularly web-applications. Fill in the technologies specific to your project and reference the corresponding standards and the process is ready to go.

Enjoy the constructive stress-free code reviews!