The purpose of a code review is to ensure a high level of code quality. A code/peer review is where developers go over the code in a system to:
1. Make sure that the code is written to standard and satisfies the specifications, requirements or design documents
2. Suggest improvement opportunities to the author
3. Learn different ways of coding and about the system under review (competence building)
Risk = probability of bug x probability of bug activation x impact of bug activation
What to Review
To get some idea which code to review, think about the following:
1. code that uses new technology, techniques, or tools
2. key architectural components
3. complex logic or algorithms
4. security related code
5. code that has many exception conditions or failure modes
6. exception handling code that cannot easily be tested
7. components that are intended to be reused
8. code that will serve as models or templates for other code
9. code that affect multiple portions of the product
10. complex user interfaces
11. code created by less experienced developers
12. code having high cyclomatic complexity
13. code having a history of many defects or changes
When starting to do code reviews for the first time, core components, base classes, and complex areas should be started with. As the code reviews progress, more coverage is possible by choosing a use case to follow through or selecting a layer. Over time, reviewing code as it is made ready and also picking on any areas that keep having bugs reported in them. However, at no point should there be a target to review 100% of the systems code.
1. While doing Code Reviews:
- Ask questions rather than make statements: A statement is accusatory. "You didn't follow the standard here" is an attack—whether intentional or not. The question, "What was the reasoning behind the approached you used?" is seeking more information. Obviously, that question can't be said with a sarcastic or condescending tone; but, done correctly, it can often open the developer up to stating their thinking and then asking if there was a better way.
- Avoid the "Why" questions: Although extremely difficult at times, avoiding the "Why" questions can substantially improve the mood. Just as a statement is accusatory—so is a why question. Most "Why" questions can be reworded to a question that doesn't include the word "Why" and the results can be dramatic. For example, "Why didn't you follow the standards here..." versus "What was the reasoning behind the deviation from the standards here..."
- Remember to praise: The purposes of code reviews are not focused at telling developers how they can improve, and not necessarily that they did a good job. Human nature is such that we want and need to be acknowledged for our successes, not just shown our faults. Because development is necessarily a creative work that developers pour their soul into, it often can be close to their hearts. This makes the need for praise even more critical.
- Make sure you have good coding standards to reference: Code reviews find their foundation in the coding standards of the organization. Coding standards are supposed to be the shared agreement that the developers have with one another to produce quality, maintainable code. If you're discussing an item that isn't in your coding standards, you have some work to do to get the item in the coding standards. You should regularly ask yourself whether the item being discussed is in your coding standards.
- Make sure the discussion stays focused on the code and not the coder: Staying focused on the code helps keep the process from becoming personal. You're not interested in saying the person is a bad person. Instead, you're looking to generate the best quality code possible.
- Remember that there is often more than one way to approach a solution:Although the developer might have coded something differently from how you would have, it isn't necessarily wrong. The goal is quality, maintainable code. If it meets those goals and follows the coding standards, that's all you can ask for.
What to Do If You're a Developer
The above advice is fine if you're the project or development leader who is organizing the code review, but what if you're the one who has to endure a painful code review? What can you do to make the process less painful if you're the developer who's having your code reviewed?
1. Remember that the code isn't you. Development is a creative process: It's normal to get attached to your code. However, the folks who are reviewing the code generally aren't trying to say that you're a bad developer (or person) by pointing out something that you missed, or a better way of handling things. They're doing what they're supposed to be doing by pointing out better ways. Even if they're doing a bad job of conveying it, it's your responsibility to hear past the attacking comments and focus on the learning that you can get out of the process. You need to strive to not get defensive.
2. Create a checklist for yourself of the things that the code reviews tend to focus on: Some of this checklist should be easy to put together. It should follow the outline of the coding standards document. Because it's your checklist, you can focus on the thing that you struggle with and skip the things that you rarely, if ever, have a problem with. Run through your code with the checklist and fix whatever you find. Not only will you reduce the number of things that the team finds, you'll reduce the time to complete the code review meeting—and everyone will be happy to spend less time in the review.
3. Help to maintain the coding standards: Offer to add to the coding standards for things discussed that aren't in the coding standards. One of the challenges that a developer has in an organization with combative code review practices is that they frequently don't know where the next problem will come from. If you document each issue into the coding standards, you can check for it with your checklist the next time you come up for code reviews. It also will help cement the concept into your mind so that you're less likely to miss opportunities to use the feedback.
Links:
The information above is gathered from the Links below:
Effective Code Reviews Without the Pain - http://www.developer.com/mgmt/article.php/3579756
General Code Review Guidelines - http://openmrs.org/wiki/Code_Review_Checklist
General Code Review Guidelines - http://ncmi.bcm.tmc.edu/homes/lpeng/psp/code/checklist.html
Macadamian's Code Review Checklist - http://www.macadamian.com/index.php?option=com_content&task=view&id=27&Itemid=31
C# code review checklist - http://weblogs.asp.net/tgraham/archive/2003/12/19/44763.aspx
SQL Server Code Review Checklist - http://www.mssqltips.com/tip.asp?tip=1303
HPROF to tune Java Application Performance - http://java.sun.com/developer/TechTips/2000/tt0124.html
2. C++ Code Review CheckList
Classes
1 Does the class have any virtual functions? If so, is the destructor non-virtual?
Classes having virtual functions should always have a virtual destructor. This is
necessary since it is likely that you will hold an object of a class with a pointer of a lessderived
type. Making the destructor virtual ensures that the right code will be run if you
delete the object via the pointer.
2 Does the class have any of the following:
Copy-constructor
Assignment operator
Destructor
If so, it generally will need all three. (Exceptions may occasionally be found for some
classes having a destructor with neither of the other two.)
Deallocating Data
1.Are arrays being deleted as if they were scalars?
delete myCharArray;
should be
delete [] myCharArray;
2. Does the deleted storage still have pointers to it?
It is recommended that pointers are set to NULL following deletion, or to another safe
value meaning "uninitialized." This is neither necessary nor recommended within
destructors, since the pointer variable itself will cease to exist upon exiting.
3. Are you deleting already-deleted storage?
This is not possible if the code conforms to 6.2.2. The draft C++ standard specifies that
it is always safe to delete a NULL pointer, so it is not necessary to check for that value.
If C standard library allocators are used in a C++ program (not recommended):
4.Is delete invoked on a pointer obtained via malloc, calloc, or realloc?
5.Is free invoked on a pointer obtained via new?
Both of these practices are dangerous. Program behavior is undefined if you do them, and such
usage is specifically deprecated by the ANSI draft C++ standard.
Constants
1. Does the value of the variable never change?
int months_in_year = 12;
should be
const unsigned months_in_year = 12;
2. Are constants declared with the preprocessor #define mechanism?
#define MAX_FILES 20
should be
const unsigned MAX_FILES = 20;
3. Is the usage of the constant limited to only a few (or perhaps only one) class?
If so, is the constant global?
const unsigned MAX_FOOS = 1000;
const unsigned MAX_FOO_BUFFERS = 40;
should be
class foo {
public:
enum { MAX_INSTANCES = 1000; }
...
private:
enum { MAX_FOO_BUFFERS = 40; }
...
};
If the size of the constant exceeds int, another mechanism is available:
class bar {
public:
static const long MAX_INSTS;
...
};
const long bar::MAX_INSTS = 70000L;
The keyword static ensures there is only one instance of the variable for the entire
class. Static data items are not permitted to be initialized within the class declaration, so
the initialization line must be included in the implementation file for class bar.
Static constant members have one drawback: you cannot use them to declare member
data arrays of a certain size. This is because the value is not available to the compiler at
the point which the array is declared in the class.
Links
"Code Review Checklist" by Charles Vaz - http://charlesconradvaz.wordpress.com/2006/02/16/code-review-checklist-2/
Code Inspection Check List - http://www.chris-lott.org/resources/cstyle/Baldwin-inspect.pdf
C Code Reviw Guide - http://casper.ict.hen.nl/se/SEscripts/CodeReviewGuide.html
Best Practices: Code Reviews - http://msdn.microsoft.com/en-us/library/bb871031.aspx
3. 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
