Reviewer's Cheat Sheet

Owned by Brendan Hofmann
Last updated: Oct 17, 2016 by Eric Lessard (Deactivated)
2 min read

In no particular order, these are smells to watch out for when reviewing your own or others' code.

  • Always specify concrete implementations with the abstract factory pattern. For example, when working with XML libraries.
    • Never rely on Factory.newInstance, because then you are at the mercy of the classloader. Always specify the implementation you want to use, either in Factory.newInstance(ImplClass, classloader) or in a system property or config file.
  • APIs should be double-checked for internal packaging and experimental headers. This is especially true for new APIs.
  • Good OO design
    • Most business logic should be pure POJOs. Do not couple to OSGi details.
    • Modules should be loosely coupled, especially across codebases.
    • Interfaces should be clear and well-defined. Strongly-typed, well-commented, with unambiguous variable names.
  • Apply defensive coding practices, especially in any code that is accessible from outside the system.
  • Unit tests should test behavior, not implementation. POJOs that cannot be fully exercised through their interfaces are usually in need of rewrite.
  • Code should be self-documenting as much as possible.
    • When in doubt, comment.
  • Question the names of variables, methods, and classes. Naming things is hard, but it's better to get it right the first time than it is to deal with refactoring later.
  • Any code that touches a core area, such as CatalogFramework, Platform, or Security, should be even more carefully scrutinized, as these areas can impact the entire system.
  • Favor convention when architecting new functionality. Code that can't be explained using design patterns is often questionable at best.
  • Avoid Not Invented Here Syndrome. We rely heavily on open-source suites like Apache and Guava; utilize them wherever possible.
  • Check for maven dependency conflicts.
  • Excluded bundle imports are a smell; fix the problem by excluding the offending transitive maven dependency instead. This keeps test and runtime environments in sync.
  • Keep the following in mind when reviewing:
    • Architecture: Does the design fit inside the overall architecture?
    • Design: Does the design follow the 5 basic OO design principles (SOLID)?
    • Reuse: Can common code be reused or extracted?
    • Concurrency: Will the code be executed by multiple threads? If so, is it thread safe?
      • Any POJO exposed as a service can be called by multiple threads. Design for concurrency unless you can prove the opposite.
    • Scalability: Can the code run in a horizontally scaled environment?
    • Readability: Is the code easy to read and understand?
    • Maintainability: Will the code be easy to maintain in the future, i.e., will a simple change cause ripple effects, a simple configuration change require a patch, etc.?
    • Administration: Is the code administrator friendly? Are log and exception messages clear and useful?
    • Migration: Any new or existing non-OSGi configuration files that need to be handled in the migration code?
    • Security: Does the code fit within the application's security framework? Are the external inputs validated? Have threat models been properly updated?
    • Performance: Are there any glaring performance problems or wasteful resource usage?
    • Test: Have the proper unit and integration tests been added? Are the tests treated as production code and follow the code review guidelines mentioned above?
    • Documentation: Have all the appropriate documents (administration, integration, developer's, etc.) been updated?