Self Code Review Methodology

When I worked as a team leader I didn't want any code to get into the product without the code review in addition to the unit tests added. But good code review takes time, so I started thinking how I can optimize the process...

First, I came to a conclusion that a review actually consists of two parts:

• Code logic. This is best done by challenging. If the developer quickly and correctly answers to questions, he probably thought about that scenario and covered it. If not, we open the relevant code and check its logic in depth. This gives the opportunity to concentrate on less 'polished' code, analyze it deeper and correct more issues.
• Code sanity, which includes:
  • Code format - ensured by IDE.
  • Code style - which actually is a routine check list. Bingo! Why not have this check list written and given to the developers to do themselves to save the reviewer's time?

Below is the check list I created, please leave your comments...

For each added field

1. Consider type safety. I.e. when the field is used, no cast is required.
2. Ensure its access modifier is private.
3. Ensure its value cannot be computed by other means (using other fields/methods). If yes, it means this field is for caching purposes or used by some setter. If it's for caching:
   • Ensure this caching is required and comment this in code.
   • Ensure it is always in sync with computed value and comment how this is achieved in code.
4. Consider marking this field 'readonly'. If not possible, consider refactoring resulting this field becomes 'readonly'.

For each added property

1. Ensure it has a getter.
2. Consider removing a setter.
3. Ensure set/get parity, i.e. if some value is set, the same one is get.
4. Ensure sequential gets return a same logical value.

For each added property/method

1. Consider minimizing its access scope (private static <--> public virtual)
2. Ensure all the arguments are checked and relevant exception are thrown.

For each added class

1. Consider minimizing its access scope (private sealed inner class <--> public not sealed).
2. Ensure it has a minimal set of constructors.
3. Consider refactoring to have one constructor with initialization logic and other forwarding to it. If needed, add a private constructor.
4. If there is an override for Equals/Hashcode methods, ensure they come in pair and both are computed from the same data.

For each local variable

1. Consider type safety. I.e. when the variable is used, no cast is required.
2. Minimize variable scope.
3. Ensure proper clean up in finally blocks; usually initailization should be right before the try block and not inside it.

For each added line of code

1. Consider implicit impacts (boxing, objects creation, computation).
2. Ensure that every System.SystemException derived exception can be possibly thrown is by intention. (NullReference, Cast etc).

For each 'lock' statement

1. Consider using a framework class, which has the required synchronization built-in. For example, if you need a synchronized Hashtable, don't do locking yourself, but create it with Hashtable.Synchronized() / SynchronizedKeyedCollection<K, T>.
2. Otherwise ensure the design is discussed with your manager.