Code Review Principles

If you think a bit, code review is an overhead. Does it add any value to the customers? The answer is no. If you tell your customers that you require 50hrs for the code reviews, customers will ask you back why do you do code reviews? Why don't you write the code right the first time? Lets answer another question. Does code review add value to the company? Hm... may be yes but very less. The bottom line is that the code reviews are not for our customers but for us. Certain companies impose code review as a mandatory process in software development processes. These companies find little value in that. According to me the code reviews are essential due to several factors such as different experience levels of developers, newly joined employee is modifying some code but he does not understand the entire system yet etc. There are different types of code reviews like self review (that every developer does), peer review (code reviewed by your co-workers) and code review by an expert who has an understanding about the entire system. In this post, I am going to talk about the best practices, guidelines for the code reviews and touch upon something called assumptions.

What are the types of code reviews?

There are two types. (1) Static and (2) Logic. Static code reviews ensures that the naming conventions are followed or not, syntax checks, common mistakes etc. And today, the static code reviews are done by the tools since its more of standard rules. Some examples of static code review tools are FindBug, PMD, CheckStyle etc. For the second type of code review, i.e., Logic, the program logic is reviewed such as logical errors, code smells, requirements to design and design to coding defect review, dead codes etc

What is dead code and how dead code is identified?

Dead code is not reachable code. The code is simply present in the source files but does not have any execution path, in other words there is no test case that can be defined to test the code. Hence the dead code is identified and eliminated during the code reviews. Identifying the dead code is tricky unless there are 100% test cases written. Code coverage reports are one way to find dead code however as I said there must be hundred percent test coverage. Just by looking at the references for a given method in the entire source project does not suffice. Some methods are not at all referenced but still get called via reflection mechanisms. And there will be callback methods, callbacks get called based on certain events.

Some examples of dead code are: int n = 3+1; if (n==5) { //some code }, unused variables. Best way to identify these kind of code is to look for warnings given by the IDEs. We may have to increase the warning level in the IDEs (Every IDE has the option of increasing the warning level) to find out all possible code warnings. By default some warnings are ignored by the IDEs.

What else to look for in the code review?

Global states: Typically global variables are, singletons, static variables are the global states in the code. The problems with the global states are (1) Non-locality -Global states can be read and modified by any other part of the program making it difficult to remember or reason about every possible use and side effects. So change at one place can affect a seemly unrelated code (2) Implicit Coupling - Between global state and other modules. (3) Concurrency Issues - Any change in the global states affects all the concurrency issues.  So minimize the use of global states

1. No global variables
2. Minimize class static variables
3. Minimize singletons

Magic Numbers: Consider the declarations such as int TWENTY_FIVE = 26, int ONE = 1 etc

Code Smells: Intuitive feeling that something is wrong with the code is what is called code smells. Highly experienced and knowledgeable developers have a feeling for good design. They routinely practice good design without thinking about it too much.

The following diagram summarizes the effective code review (Click on the image to zoom)

 

ASSUMPTIONS

There are two types assumptions while developer is coding. (1) Implicit Assumption and another (2) Explicit Assumption. Assuming something during while code development will definitely have side effects. Lets take a simple example to better understand the types of assumptions. Say a developer codes the instruction as char filename[256]. Here it is assumed that file length is always up to 256 characters and this is an example of explicit assumption. And this also has a side effect, this code works only with English Operating Systems, when the code has to run in Japanese OS, this will crash since it does not implement Unicode and this is an example for implicit assumption.

Typical implicit assumptions

1. User is expected to call function X() only once.
2. User is expected to call function X() before calling function Y().
3. There are no side effects of overwriting the value of Z
4. Assumptions about underlying OS (Supported OS, file name sizes, path separators etc)