<html><head><title></title></head><body><div style="float: right">
| Table of Contents | ||||||||
|---|---|---|---|---|---|---|---|---|
|
</div>
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 and (2) designing.
Back to Code Review Overview
- coding
- designing
Maintainability
- 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.
- Does the code comply with the accepted Coding Conventions (provide link)?
- Indentation
- Variable / method names
- Bracket style
- Does the code comply with the accepted Best Practices?
- See OpenMRS Conventions
- 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†"work around" an issue should be thoroughly commented, so as to avoid confusion and re-introduction of bugs.
- Code that has been “commented out†"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.
...
- Does the code comply with the accepted Exception Handling Conventions.
- We need to expand our notion of Exception Handling Conventions.
- Some method in the call stack needs to handle the exception, so that we don’t don't display that exception stacktrace to the end user.
- Does the code make use of exception handling?
- Exception handling should be consistent throughout the system.
- Does the code simply catch exceptions and log them?
- Code should handle exceptions, not just log them.
- Does the code catch general exception (java.lang.Exception)?
- Catching general exceptions is commonly regarded as “bad practiceâ€Â"bad practice".
- Does the code correctly impose conditions for “expected†"expected" values?
- For instance, if a method returns null, does the code check for null?
- The following code should check for null:
- For instance, if a method returns null, does the code check for null?
| Code Block |
|---|
|
...
Person person = Context.getPersonService().getPerson(personId);
|
...
person.getAddress().getStreet(); |
- What should be our policy for detecting null references?
- Does the code test all error conditions of a method call?
- Make sure all possible values are tested.
- Make sure the JUnit test covers all possible values.
...
- 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.
...
- 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.
- Does the code avoid deadlocks?
- I’m 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.
...
- Is the code as generalized/abstracted as it could be?
- 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.
Panel
- 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.