Reviewer's Cheat Sheet

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)?
    • Single responsibility principle (SRP)
    • Open/closed principle (OCP)
    • Liskov substitution principle (LSP)
    • Interface segregation principle (ISP)
    • Dependency inversion principle (DIP)
  • 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?