Ever controversial Code Reviews

A long haul in posts (at least by my standards) was basically due to combination of work schedule, “other things” (don’t ask ). For past couple of weeks I have had my code reviewed by some people, which interestingly evoked emotional response in me to write this post. I have been around long enough (hopefully ) to know what code reviews can do to a developer whose code is being reviewed. More often than not, it turns out that code reviews are a one-on-one sparing match between reviewer and the developer him/herself.

Keeping in my traditional approach, lets define what a code review is, (taken from Wikipedia off course):

Code review is systematic examination (often as peer review) of computer source code intended to find and fix mistakes overlooked in the initial development phase, improving both the overall quality of software and the developers’ skills.

Types of Code reviews :

1. Fagan Inspections:

   This was more or less formalized by Michael Fagan. The essence of Fagan inspections lies in comparing output of minuscule operation to the so called “exit-criteria” already defined or comparing with output requirements as defined earlier in requirement gathering process. This kind of review makes a lot of sense in keeping the code on track for the end result software product. More often than not, in their ever increasing quest for developing “cool” software developers tend to overlook requirements specifications. The idea is to verify (at each step in an iteration) functional aspects of what is being developed, assigned to individual developers. For example if Sam is working on “search” aspect of the system (I am using Non Programming language on purpose), Is he following what is “required” ?, Is the requirement being “met” by what he has written? … by asking those kinds of questions and validating the code; the end product becomes bug resistant (It obviously can never be Bug free ). The important piece of the puzzle is to do this in smaller iterations and not at the end of development iteration, where developer has worked on a size able block of code already. If the code written is working and is meeting the functional requirements, it seems that fagan inspections would pass through. In “real” world this is off course only one side of the story. More often than not the requirements are not the ONLY factor in doing inspections; sometimes inspections can yield a “possible” crack through in the code which off course leads to more rework. A typical Inspection process involves Planning —> Overview —> Preparation —> Inspection —> Rework —> Follow up.

2. Informal / Traditional Code reviews :

   Developers meet one on one, and sit down side by side, to inspect each line of code, and review happens by asking questions; like “Why is this being done in this way and not “that” way? … Often these kinds of reviews evoke emotional responses from both the developers involved. ( for more read this great article : Code Reviews without much pain!). In most cases developers involved are emotionally involved with the code that they write, hence it becomes very important to criticize their code very carefully and not to go overboard about the cynicism. As you would read in the link, More often a senior developer will be reviewing a junior developer’s code, if that is the case, it is of paramount importance that senior developer shoulder’s the responsibility of reviewing code in a “constructive” manner drawing a line between what is “required”, what is “right” way, what is officially “correct” way of doing things. In many cases, (as I have seen in the past and seeing now) developers will often be asked to rework their stuff for the sake of being “consistent”. Understandably that makes the code more readable and maintainable, but more often developers will be reluctant to refactor their code to more OO unless it is absolutely necessary, therefore when i say “Careful” i really mean careful in the sense that review should not hinder the overall project time lines. It is very easy to sit there and be an architect stating what could have been done in a better way, than actually refactoring it (by refactoring I meant making it Object oriented yet not breaking functional aspects of the system). Utmost care has to be taken to pass on judgments on what others are writing or doing. In XP world, code reviews form an essential aspect for each iterations to allow a healthy product to move forward. Under pressure deadlines, it is very important to have a few quick sessions (without laying out all the things that “You” want as opposed to what “ALL” people would agree upon) in which decisions are taken about what is acceptable and what is not. It is VERY VERY important that decision is made by keeping at least 80% of the team happy. This I am speaking from experience; Before my current project, the place where i worked at, code reviews were done at a very late stage, there was an excel sheet that was sent out to the entire team to make changes at Class level. (stating Line Number, methods where the problem is). This is obviously not a good approach, since it can be very annoying for developers who will have to refactor their code at such a late stage in the game, when “functional” aspects are already covered.

Needless to say, code reviews form an important aspect in software development. In doing code reviews aside from emotional aspects, utmost care has to be taken to avoid being “obsessive” about style of the code. You can enforce a formatting standard by using common XML or something similar ( depends on platform, I am speaking from Eclipse /IDEA/ Rational Application Developer perspective off course), but that should be the end of it. I have seen this happen in many code reviews where some people prefer somethings written in some ways , for example I usually prefer my If-ELSE constructs leave a line in between (I took this approach from someone just a year back, because that made my own code readable); Now this still adheres to the coding standards / format of the common formatting XML, but reviewers who have their own style of writing the code will often want to enforce that style in the code that they are reviewing. This is obviously a nuisance in the code review process. As i said earlier, utmost care has to be taken to draw a line as where you would want to stop and look at what would be advantages of doing things in such a way. In my experience, If a developer writes code in a certain way, but is a stickler for certain types of constructs (still adhering to a common formatting standard), he/she should be allowed to be at that. Enforcing things that “YOU” as a developer want can prove to be decisively spoiling relationships in team dynamics. (I have had this kind of experience in almost every project now, where a senior developer wants code to look in a certain way and then he goes over board in enforcing it.).

Code inspection and reviews can be enforced with some of Version Control systems. Especially with products like “All Fusion Harvest Change Manager” (now it is renamed to something else), where packages are assigned to be promoted to other stages in a project (for example DEVELOPMENT —> QA / TEST stage) only when they have passed code reviews assigned by developers. Again, developers developing the code might want to check in their code and test it out on the live server without wasting their time waiting on code reviews (if the reviewer is not available), in such cases , they would still be able to do so with systems like Harvest. In my experience and opinion version control system should be used for checking in “functional” code until you reach a logical point where things are working. A non working code or breaking code checked in can be hazardous to the team. In projects where maven is used, Test First (TDD) approach can be very annoying. especially when developers write test cases which off course are broken because the functional pieces for which tests were written are not in working condition. So a simple mvn clean install (used for building projects, more on Maven later). would not work for the entire team, because test cases are executed and they would be fail, off course one can skip the tests altogether but still. In this respect, a checked in functional piece, however small it might be, can be guaranteed to work since it passes the test. In such cases a code review for such piece of code becomes irrelevant, unless off course as i said in my previous passages people want to play one-on-one tag matches with each other.

Teams working on projects with stringent deadlines often tend to overlook importance of code reviews. Code reviews can go a long way in eliminating certain kinds of errors / bugs and make code look more consistent and maintainable, provided off course a well thought off plan is executed. Cynics have often pointed out to me that in XP world, code reviews are not needed at all, since doing pair programming can eliminate the need for that. True only for 1% times in my opinion. I am not a pair programming basher, but i do feel sometimes that senior developers will often do almost most of the coding or thinking part, which means that junior developers will not think or grow while writing their code. Constructive pair programming means listening to others and pointing juniors out in right direction … This off course is too generic or subjective and almost always doesn’t work. In my experience, pair programming will only help those developers who are “just starting out”, for people who have been working for sometime, if and when paired with people who have been in the business for years can be disastrous. Disastrous in the sense that it can often create friction between two passionate programmers who know their stuff and turn pair programming into knowledge matches; this is obviously not good for the team and for the future efforts.

All in all code reviews form crux of every organization, they help in fixing issues that might arise in maintenance phase which off course are costlier and time consuming to fix than in development phase itself. Beyond not meeting functional requirements, code reviews can help in finding out potential performance bottle necks as well.