Code review should focus on seeing that coding conventions are being followed and basic design decisions are sound. As a general rule, two things should NOT happen during code review:

1. coding
2. designing

Things to look for during a code review

What not to do

1. (re)designing the architecture
2. commenting on style alone (unless not adhering to Code Style
3. writing code (only applicable to Group Code Reviews)

What to look for in the code

Overview

1. Finding bugs
   • NPEs, no privilege checks, etc
2. Create common Coding style

Maintainability

1. Does the code make sense?
   • Make an effort to understand what the code is supposed to do before performing a code review. This can also be a part of the code review.
   • Require the developer to comment as much as necessary to make the code readable.
   • During code review, it may be necessary for lead reviewer to make comments in the code, with help from original developer.
2. Does the code comply with the accepted Coding Conventions (provide link)?
3. Indentation
4. Variable / method names
Bracket style
5. Does the code comply with the accepted Best Practices?
   • See Conventions
6. Does the code comply with the accepted Comment Conventions?
   • All classes and methods should contain a descriptive JavaDoc comment.
   • All methods should contain brief comments describing unobvious code fragments.
   • All class files should contain a copyright header.
   • All class files should contain class comments, including author name.
   • All methods should contain comments that specify input parameters.
   • All methods should contain a comment that specifies possible return values.
   • Complex algorithms should be thoroughly commented.
   • Comment all variables that are not self-describing.
   • Static variables should describe why they are declared static.
   • Code that has been optimized or modified to "work around" an issue should be thoroughly commented, so as to avoid confusion and re-introduction of bugs.
   • Code that has been "commented out" should be explained or removed.
   • Code that needs to be reworked should have a TODO comment and a clear explanation of what needs to be done.
   • When in doubt, comment.
   • When you've commented too much, keep commenting.
   • When your wrists hurt from commenting too much, take a break ... and then comment more.

...

1. Does the code appear to pose a security concern?
   • Passwords should not be stored in the code. In fact, we have adopted a policy in which we store passwords in runtime properties files.
   • Connect to other systems securely, i.e., use HTTPS instead of HTTP where possible.
2. Service methods should have an @Authorize annotation on them
3. JSP pages should use the openmrs:require taglib

Thread Safeness

1. Does the code practice thread safeness?
   • If objects can be accessed by multiple threads at one time, code altering global variables (static variables) should be enclosed using a synchronization mechanism (synchronized).
   • In general, controllers / servlets should not use static variables.
   • Use synchronization on the smallest unit of code possible. Using synchronization can cause a huge performance penalty, so you should limit its scope by synchronizing only the code that needs to be thread safe.
   • Write access to static variable should be synchronized, but not read access.
   • Even if servlets/controllers are thread-safe, multiple threads can access HttpSession attributes at the same time, so be careful when writing to the session.
   • Use the volatile keyword to warn that compiler that threads may change an instance or class variable - tells compiler not to cache values in register.
   • Release locks in the order they were obtained to avoid deadlock scenarios.
2. Does the code avoid deadlocks?
   • I'm not entirely sure how to detect a deadlock, but we need to make sure we acquire/release locks in a manner that does not cause contention between threads. For instance, if Thread A acquires Lock #1, then Lock #2, then Thread B should not acquire Lock #2, then Lock #1.
   • Avoid calling synchronized methods within synchronized methods.

...

1. Does the code make use of infinite loops?
   • If so, please be sure that the end condition CAN and WILL be met.
2. Does the loop iterate the correct number of times?
   • Check initialization and end condition to make sure that the loop will be executed the correct number of times.

Performance

1. TBD

Functions

1. TBD

Bug Fixes

1. TBD

ReusabilityReusability

1. Are all available libraries being used effectively?
2. Are available openmrs util methods known and used?
3. Is the code as generalized/abstracted as it could be?
4. Is the code a candidate for reusability?
   • If you see the same code being written more than once (or if you have copied-and-pasted code from another class), then this code is a candidate.