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?