Code Review Redux

James Turner recently interviewed Alex Martelli at the O'Reilly Open Source Convention (OSCON). Alex is well known in the Python community. He is also a tech lead at Google. The subject of the interview was Code Reviews, which you know I have an interest in. So I thought I would go over and comment on the Alex's points.

The purpose of a code review is to catch errors. However a byproduct of reviews is also for developers to learn. Some things a code review can accomplish are to fix mistakes, ensure a uniform style of coding, check for good variable names, and factor our common functionality.

Obviously there are some shortcomings to the current method of conducting code reviews. Junior developers get intimidated by senior ones, especially when the old salts have big egos. In other scenarios the developers say nothing during a review. They have the attitude that they want to get out as soon as possible, so they state that everything looks fine. At the other end of the extreme are developers who nitpick every little thing they can with the code reviewed. Finally there is a lack of good tools to help code reviews.

Alex had some advice on how to improve the code review situation. He recommends that reviewers ask questions instead of criticize the developer. All people involved with code review should treat the activity as a way to learn. It helps immensely if the developers are humble and have integrity. And using software to assist the process is valuable. A tool which allows a reviewer to read the code, click points of interest, enter comments, and respond to comments is invaluable.

Alex also provided some examples of code review from the commercial community. Apaches uses an empty bug list as the signal to enter code review. Test Driven Development (TDD) seems to be taking the place of code review at many commercial development firms. However such techniques will not find some subset of problems such as race conditions and/or deadlocks. Pair programming also replaces code review in other locations. But after two developers have paired up for a while, it becomes hard to conduct independent review.

Prior to the interview, I had never heard of Alex Martelli. However I decided to listen to the video interview since he worked for Google. And I had respect for him having heard the insight he provided on code reviews. In closing he had mentioned the use of defect injection, where defects are randomly introduced into a system. The code review process is then judged by how many such defects are caught. Perhaps I shall try to introduce this technique into our own software development team. It might be a starting point to fix the movement away from code reviews on my project. I will let you know how that goes.

Document Your Work

We have an oral process on my project that you must document your progress on trouble tickets in our bug tracking system. Recently a lot of the development staff have not been following the rule. This causes a lot of overhead, especially for members of the Help Desk. It is a pain to have to document your every step in a defect tracking system. When you are on the hunt for the source of a problem, you want to keep focused on the problem at hand. Documenting progress may slow you down and cause you to lose your focus.

There are some benefits to documenting your progress. It prevents other people from having to call you up or e-mail you to find your status. This prevention has a rippling effect. If the customers can view your progress, they do not have to talk to people who will only talk to other people to get to the answer. Sometimes just the evidence that somebody is making updates helps calm a customer that is encountering a lot of problems.

I work on a lot of trouble tickets. And I like the work. Personally I make sure I log my progress in frequent intervals. I make sure I write my findings for a broad audience. However I also add enough detail so that at any point, another developer can come in, read my comments, and continue the research. This has made redistribution of work very easy for the people I work for and with.

Another incentive for writing down your progress in the defect tracking system is that it can lead to less meetings. That means less meetings amongst developers. It also means less meetings with the customer. When somebody is not confident that progress is being made on trouble tickets, meetings are called to order. This is a vicious cycle. The meetings take up time and prevent you from actually fixing the bugs. Or they make you stay late when there are no meetings to find out what the problems are.

Right now our help desk has to attend trouble ticket meetings. The help desk personnel are trying to rally developers to enter their updates in the bug tracking system. I think I will join them and spread the word. Document your work already. You will only be helping yourself.

Code Review

The Little Tutorials web site has a recent entry that takes a contrarian point of view for Code Reviews. The author concludes that they are mostly a waste of time. He contends that people are already busy enough, and that forced code reviews frequently degrade to something unproductive. The author does make some recommendations if you absolutely must conduct code reviews. You should run your code through the debugger first before presenting it for peer review. I share some of the sentiments from this article. But I also have some code review success stories that sway me a different way in the end.

I work on a rather complex business application suite at work. We have, for the most part, subjected maintenance changes to the code to code reviews. This has indeed come at a high development cost. But the reviews we perform are not your normal type where a lot of developers get together in a room to go over the code. Ours are conducted independently by one other member of our dev team. The author puts together documentation that show all the code changes. The author also produces a unit test plan. These documents are reviewed. Comments are entered on a standard peer review sheet. The result is sent back to the author via email.

The surprising fact is that I have found our code review to be very useful. But the effect may not be as you would expect. Just the fact that we must prepare for a code review influences the quality of the work that is performed. If you know that you must document your code changes, you become more aware of your changes as a whole. And you must thoroughly understand what you are doing. Otherwise it will come up at review time since the reviewer must be able to also understand the changes based on the documentation you product. This works the same way with development testing. Simply the act of having to write down a unit test plan forces the developer to actually think about, plan, execute, and write down the steps steps and scenarios.

Our code reviews occasionally also produce the positive effect that you might expect a normal review to do. Some developer who did not understand the true requirement for the change gets enlightened. A developer gets encouraged to add more comments to promote maintainability of the code. A reviewed finds alternate paths that needs to be unit tested before the fix gets shipped.

Exceptions to the Rule

I recently read a blog post by Chadran Nair emphasizing the occasional need to override the existing process when circumstances dictate it. In fact, the author went on to say that companies which understand and implement this policy get his business. I wonder whether this is applicable in the software development realm. I do not think processes are to be taken lightly. And the breaking of normal process should also be a serious and seldom used resort.

We had a process to compute statistics for how well our software did each year. A couple years ago our customer requested the process be followed to produce input for an important report. This task came to me. So I followed the process and got some unusual stats. They were way too big. I could not imagine how we were coming up with the numbers I was getting. The process dictated that I follow the rules and produce the results.

I inquired to my manager whether I should blindly follow the process and publish the wild results. Like most managers at the time, my manager decided to protect himself and told me to work it out with our customer. Now I had a good relationship with most members in the customer community. So I went to the top technical man in our customer organization. I shared my concerns with him. He definitely told me to research the numbers and find out why the results were off the charts. I was able to skirt by the normal process and found a data point that was messing up all the statistics. In the end, I was told to report a set of numbers that blindly followed the process, as well as another set which discounted the wild data point.

Perhaps this example illustrates the right way to decide when to bypass the normal processes. It should not be done in isolation. And it should be done sparingly. You should get management (or customer) buy in first. But there comes a time when reason needs to prevail. Once in a while you cannot be a drone and follow the documented process when it is going to lead you astray. Yes you can defend yourself by stating you followed the process. However as the old saying goes, rules are meant to be broken.

Change Control

Our system has a huge database of information that it stores. Some database tables have 100 million plus rows. And there are hundreds of database tables used. Development makes changes to the schema throughout the year. These changes need to be tightly controlled to prevent chaos from spreading.

In the past, the DBA team implemented a process to control all changes to the database. Requests would be made for the changes. All the teams would approve the changes. The DBA team would promote the changes to development, test, and then production. Changes were grouped into releases to minimize the work to implement the changes. This arrangement worked well most of the time.

Somewhere along the line, a team of consultants came in to help re-architect the system. Unfortunately this re-architecture was a complete disaster. The customer eventually pulled the plug on the new system. Development was tasked with restoring the old system. Through some miracle the consult team remained on the project. And now they are working to take over the change management for the database.

Previously the DBA team had a good rapport with the rest of the development team. So the database change management ran smoothly. Now there is a fight for the control. There was no clear edict that appointed the consultants as the maintainers of the database. In fact, they do not actually implement any changes. The DBA Team does that. The result is that there is not a clear direction on how to initiate changes for the database.

The management team is aware of the current situation. And they have their own objectives to achieve on this project. At about this time every year, we need to start making monthly changes to the database in preparation for next year’s release. The database change process needs to be ironed out and the control issues resolved before we can perform this task properly. It seems that politics is getting in the way of technical progress once again.

Documentation

Our team currently has a task to update our design documentation based on the changes we made when migrating to a new tool set. The amount of documentation change is small. Our goal was to ensure that functionality remained unchanged during our tools upgrade. We only had to modify one small part of report invocation. Unfortunately we found out that nobody knew the process to perform the update.

Part of the confusion stems from the fact that the software maintenance is being performed by a new company that recently won the contract. In addition, the customer decided to become more involved with the documentation management. This just adds complexity to an already complicated task. Previously the development team used to make design changes in Rational Rose. We would use Rational SoDA to extract design information from Rose to produce the design document. But SoDA has some problems which we were unable to resolve. So the process changed to involve SoDA generation with some additional markups by hand. Things got hectic when the previous company that did maintenance was wrapping up their contract. The SoDA generation was dropped and a Microsoft Word document was edited exclusively to incorporate design changes.

Currently one individual in our client organization is tasked with managing the latest version of all documents. This includes our large design documents. One would think that development could go to this individual to get the latest version of the design doc for update. But we have been cautioned that this individual sometimes gives out the wrong version of the document. Furthermore I do not think any of the procedures are documented. And if they are, the development team does not know about them. I sense a severe weakness in the maturity of the process here. This will most likely come out if we were to be audited.

I think somebody is working on these problems. This impacts the day to day work when a developer needs to document some design changes. In general, coders do not like to perform documentation tasks. So nobody is sweating this problem just yet. However we are embarking on the design for a number of new features requested by the client. Therefore things are about to get very hectic real quick in our documentation world. Good luck to us.