JAVA Code Review CheckList
Error Handling
1. Does the code comply with the accepted Exception Handling Conventions.
a. We need to expand our notion of Exception Handling Conventions.
b. Some method in the call stack needs to handle the exception, so that we don’t display that exception stacktrace to the end user.
2. Does the code make use of exception handling?
a. Exception handling should be consistent throughout the system.
3.Does the code simply catch exceptions and log them?
a. Code should handle exceptions, not just log them.
4.Does the code catch general exception (java.lang.Exception)?
a. Catching general exceptions is commonly regarded as “bad practice”.
5.Does the code correctly impose conditions for “expected” values?
a.For instance, if a method returns null, does the code check for null?
The following code should check for null
Person person = Context.getPersonService().getPerson(personId);
person.getAddress().getStreet();
What should be our policy for detecting null references?
6.Does the code test all error conditions of a method call?
a. Make sure all possible values are tested.
b.Make sure the JUnit test covers all possible values.
Security
1. Does the code appear to pose a security concern?
a. Passwords should not be stored in the code. In fact, we have adopted a policy in which we store passwords in runtime properties files.
b.Connect to other systems securely – i.e. use HTTPS instead of HTTP where possible.
Thread Safeness
1. Does the code practice thread safeness?
a. 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).
b. In general, controllers / servlets should not use static variables.
c. 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.
d. Write access to static variable should be synchronized, but not read access.
e. 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.
f. 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.
g. Release locks in the order they were obtained to avoid deadlock scenarios.
2. Does the code avoid deadlocks?
a. 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.
b.Avoid calling synchronized methods within synchronized methods.
Resource Leaks
1. Does the code release resources?
a. Close files, database connections, HTTP connections, etc.
2. Does the code release resources more than once?
a. This will sometimes cause an exception to be thrown.
3. Does the code use the most efficient class when dealing with certain resources?
a. For instance, buffered input / output classes.
Miscellaneous:
1.Make sure that we are using StringBuffer if we want to change the contents of a String
2.Always use “.equals” instead of “==” during Object Comparision
3.Use wait()/notify() instead of sleep()
Links:
Checklist: Java Code Review - http://snap.uci.edu/viewXmlFile.jsp?resourceID=1529
http://www.javaworld.com/javaworld/javatips/jw-javatip88.html
http://undergraduate.csse.uwa.edu.au/units/CITS2220/assign2/JavaInspectionCheckList.pdf
http://www.cs.toronto.edu/~sme/CSC444F/handouts/java_checklist.pdf
http://www.deaded.com/staticpages/index.php/codereviewprocess
