A while ago, one of my colleagues (Chi Wai Man) asked about my experience in code reviews, or as I would prefer to call it, code inspections. He is assigned to a project at one of our customers and, triggered by my "This thing is huge post" conducted a little review on the source code in that project. Based on what he then saw (code complexity figures exceeding 50, no code standard) and starting a new project to move from ASP and VB6 to ASP.Net and VB.Net, he wanted to start a code inspection practice in that project. You can read about that here.
If you're a regular visitor of this web log, then you're probably aware that I believe that code inspections can improve the quality of the deliverables. Mr. Man's question triggered me to think about code inspections and how I think this process could (or should) take place. The result is this post, in which I will try to explain how you might setup a formal process to conduct code inspections for .Net applications. It has become quite a large post, my apologies for that. Feel free to comment!
Before I start...
The process I will try to explain is based on the formal process first described my Michael Fagan in 1976. I have experience in conducting code inspections using that formal process at Exact International Development, my previous employer. I helped introduce this process in the Dutch branch of the company and acted as a moderator and inspector on numerous inspections. I was also fortunate enough to be able to introduce this method at other companies while working for LogicaCmg, my current employer.
You may also want to read my views on software development in general and coding standards in particular. The views expressed in that post have influenced the way I look at the code inspection process. Everyone is talking about RUP, MSF, SOA and SOD to just name a few hot development methods and architecture in today's IT business. All claim to help build better software. Yet none of them provided methods to help you improve the quality of your code. And they shouldn't, probably...
A lot of what you'll read in this post is based on past experience and various other resources. Links to related resources are summarized at the end of this post. If you have anything to add, then please let me know. Any suggestions and/or corrections are greatly appreciated. Please keep in mind that I am not explaining or promoting the Fagan process. So if you think stuff is missing or different when you compare it to Fagan methods, you're absolutely right. I use a simplified version of the process, because it makes it easier and faster to implement.
Is code inspection really that useful?
At this moment, there are so many free and commercial tools available that check your source code that you might think it is no longer necessary to check the code your self. I will be the first to admit that more and more checks on your source code can be automated. And that is a good thing. An automated check of your source code will ensure everything is checked against the same basic rules. For example: When you use Microsoft FxCop, the source code is checked for compliance to a large list of rules on various areas such as Globalization, Performance, Security, etc.
From a quality point of view
But there are problems in your code that automated tools cannot (yet) find. And it is likely that a unit test or a functional test might not find as well. A few examples:
The following code shows a method which copies the contents from one typed DataRow object to another.
static void CopyFileRow(myTypedDataset.FileRow source, myTypedDataset.FileRow target)
{ target.Id = source.Id;
target.FileName = source.FileName;
target.DateReceived = source.DateReceived;
}
The compiler will not give you any warnings on this code. FxCop will also tell you everything is fine. But is it really? What if the column DateReceived is allowed to contain a DBNull value? This method could then throw an exception with the message "Cannot get value because it is DBNull" when this is actually the case. It is this type of behavior that you might never find in a unit test or even a functional test. By conducting a human inspection of the code, you might have caught it before the application is tested. And you would have changed the code to something like this:
static void CopyFileRow(myTypedDataset.FileRow source, myTypedDataset.FileRow target)
{ target.Id = source.Id;
target.FileName = source.FileName;
if (!source.IsDateReceivedNull)
target.DateReceived = source.DateReceived;
}
Another example is the following method:
static decimal Division(int a, int b)
{ return a / b;
}
The compiler will not give you any warnings. FxCop will tell you there's nothing wrong. Yet as a .Net developer you should be aware that there are two major problems in this method. The first and most obvious problem is of course a division-by-zero problem. Most developers will immediately notice this, but I still see code like this in numerous applications. But this method will not return the expected result and that's even worse. Try running this method with a = 1 and b = 2. You would expect 0.5 as a result, but the method will actually return 0.
Now I know there are developers that will argue that they never make mistakes like this. And that only inexperienced (or unqualified) developers will make mistakes such as this one. Well, then call me stupid, because this is actually a mistake I made recently. Luckily, this was found during testing. But I wish I had found it before a tester ran into this problem. And yes, I did check the code with FxCop and I have a unit test that runs successfully. So from that perspective I argue that inspecting your code is useful.
From a cost-of-maintenance point of view
This may sound too good to be true, but consider the following scenarios
Scenario 1 - A developer finds the problem before the application is sent to test.
Developer A writes some code and tests it. When he/she finds a problem it will be solved. The code is then retested. This process repeats until the developer no longer finds any problems.
Scenario 2 - A tester finds the problem before it is sent to the user.
Tester B receives an application for test from Developer A. When a problem occurs, the problem is logged and an error description is sent back to Developer A. Developer A tries to reconstruct the problem. The problem is then solved and the code is tested. The application is then sent back to Tester B who can resume testing.
Scenario 3 - A user finds the problem while using the application
User C installs the application developed by Developer A and tested by Tester B. When a problem occurs, the user contacts the helpdesk. They will identify if it is a real problem. If so, the problem is logged and sent to the Tester B. Tester B will verify if the problem still occurs. It might have been solved already. If it is not solved, Tester B sends the error description to Developer A. Developer A then tries to reconstruct the problem. The code is changed to fix the problem and the application is sent to Tester B. Tester B verifies that the problem is solved. When this is the case, the user can redeploy the application with the implemented fix.
Needless to say that it is a lot cheaper to find problems as early as possible. Especially when a user finds a problem. the costs to solve the problem increase enormously. And there's the risk of a damaged reputation for your company. From that perspective, a code inspection process has the potential to reduce the cost-of-maintenance.
The average software projects contains about major 50 defects per 1000 lines of code. You will find this figure being mentioned in various resources. Finding these defects makes sense although it is an illusion to think that we can solve all of them. The code inspection process is one of the things you can do to get rid of as many defects as possible before you deliver the product to the end-user.
There are a number of things you gain from conducting a code inspection:
- Code inspections help reduce the number of technical defects in the code. This will improve the technical quality of the end project.
- When you have less technical defects, you have more time to find functional defects.
- Developers learn from their mistakes and from comments made by their peers. One might expect them to write better code in future projects as a result.
- Improved technical quality will reduce the time lost by testers when they run into a problem that was caused by a technical defect.
- Logging the results of the inspections will give insight in the technical quality of your projects.
- Your product quality will improve and results in a happier customer!
The basic process
Acceptance
An important requisite for an inspection process to be successful, is the acceptance of the process by the developers on the team. If you cannot get the developers to agree to this process, you will never get good results from this process. In those case, you will often see that the inspection process is abandoned, especially when the project reaches the deadline and pressure on the development team increases.
Management must also accept that the inspection process will take up time and resources. They will need to be convinced that the inspection process can improve the quality of the end product. Logging the results of the process and thereby proving that problems have been eliminated before the product reaches the delivery phase can play a vital part in convincing management. The inspection process can be seen as an investment.
Roles
A formal inspection is conducted by an inspection team. The members of this team will have the following roles:
- Moderator. Responsible for administrative tasks, disseminating inspection materials, scheduling meetings, moderating the inspection meeting, collecting data, overseeing follow-up, and issuing the inspection report
- Author. Responsible for the code to be inspected satisfying inspection entry criteria, for providing the overview of the code, for providing clarifications at the meeting, and for rework needed.
- Reader. During the meeting, leads the inspection team through the code being inspected.
- Inspector. Familiarizes himself/herself with the code to be inspected and identifies issues with it
- Recorder. Documents issues, decisions, and recommendations. Records inspection data for process analysis.
It is possible for a member of the inspection team to have more than one role. Exceptions to this are the Author and the Inspector roles. Members of the team with this role should only be concerned with the tasks assigned to that role. The Moderator could be allowed take on the additional roles of Reader and Recorder.
Inspection process
The inspection process consists of 7 steps as can be seen in this flow chart:
The process starts with selecting the code (step 1) that will be inspected. A meeting is planned (step 2) where the inspection team will discuss the results of the inspection. The Inspectors and the Author prepare (step 3) for the meeting by inspecting the code and logging all defects they find. During the meeting (step 4) the Reader leads the members of the inspection team through the code. The Moderator is also in this meeting, making sure everything runs smoothly. During the meeting, the Recorder makes sure all found defects are logged categorized. After the meeting, the Author will fix any defects (step 5) that have been designated for rework. The Author will inform the moderator (step 6) when the defects have been fixed . The moderator will then report the results (step 7).
Artifacts
The code inspection process uses a number of artifacts which are re-used throughout the entire process:
- DefectLog v1.0.XLS. This workbook is a form in which each Inspector (and the Author) log the defects they encounter.
- InspectionReport v1.0.XLS. This workbook is a form which is filled in at the end of the inspection meeting. The log will contain summary data about the inspection.
- InspectionData v1.0.XLS. The results of each inspection are entered in this excel workbook. The accumulated data can be used for reporting to management. The data also gives the inspection teams insight in the improvements they accomplish after conducting inspections.
A sample inspection data report can be downloaded here. It contains data from a project in which I introduced this method of inspections.
There are some other artifacts to consider that can play an additional role in this process:
- Code standards. Every project has one and there's no definite list. I prefer to use standards that can be checked automatically, but there are things that automated checks just can't do. That's the sort of thing I would add to a code standard. But there are so many views on this that I would suggest you use Google to find a version you can live with.
- Check lists. Based on best practices, you should keep a short-list of items to check for when you do a code inspection. Just to make sure you don't forget the most obvious and important checks. A sample for C# can be found here. It's not an ultimate list, just some things I check when I do an inspection of C# code. Check lists should always be work-in-progress. There are checks on the list that can be automated, so in time they could be replaced by new checks that cannot (yet) be automated.
The process in detail
Step 1 - Selection
Roles involved
Description
In the most ideal situation, a code inspection process is put in place when you start with a project. In that scenario, you can start inspections from the moment the first code has been written. The reality however is that code inspections are put in place when a project has been running for months and unexpected behavior is found during testing.
You will need to decide what code is likely to be a candidate for reviewing. To do this, you have the following options:
- Use a code metrics analysis tool which will give you an indication into the code complexity. There are a number of commercial and open-source options available. I have used the community version of DevMetrics in the past, but the company that created that software has stopped development and moved the project to source-forge. Compuware has a commercial product called DevPartner Professional, which I really like. There's also a Reflector Add-in that can do this. Once you know which methods are the most complex, then start inspecting the code that contains these methods.
- From the test results, it is likely that you (or the development team) can pinpoint the problems to only a select number of components. These components are a good place to start.
- Talk to the developers on the current project. They usually have a good idea which code could be a candidate for inspection.
Do not start inspecting any code until the following entry criteria are met:
- All VB.Net code must compile without problems with the following options:
- Option Strict set to On.
- Enable build warnings checked
- Treat compiler warnings as errors checked.
- All C#.Net code must compile without problems with the following options set:
- Warning level 4
- Treat Warnings as errors set to true
- All code must pass the checks that are performed by FxCop or any other code inspection tool such as Compuware Devpartner. When certain checks are ignored, then it should be documented why they are ignored.
- Technical documentation about the code must be present, describing at least the purpose and context of the code.
The entry criteria ensure that a number of obvious checks on the code have already been performed. The inspection tools check the code for naming conventions, best-practices and other common mistakes. The compiler options ensure that the highest level of checks that can be performed by the compiler have been executed. As these are all automated steps, you are guaranteed that all checks always take place and that all code pass at least these checks. Inspectors can focus on other issues.
Step 2 - Planning
Roles involved
Description
Once the code has been selected, it is time for the Moderator to plan the meeting. The code and accompanying documentation is gathered. The Moderator will check if the code meets the entry criteria. If that is the case, team members can be selected for the inspection. The team consists of at least the following members:
- The Moderator. He/she is responsible for the entire process and should be present at the meeting to ensure that meeting is conducted in a proper way.
- At lease two Inspectors. The Inspectors will check the code for defects. You should at least have two inspectors. There will be defects that both Inspectors will find. But more important, you will find that some defects are detected by Inspector A and others by Inspector B.
- The Author. The Author has to be present. He/she knows the exact context and add additional information about the code to the Inspectors.
All team members will receive the following artifacts:
- The code to be inspected.
- Technical documentation.
- Background information.
- Defect log.
There is a limit to the number of code lines that can be reviewed in a amount of time. Based on previous experience, and resources on the Internet confirm this, you get the best results when you review approximately 200 lines of code per hour. In the scatter graph to the right, you can see the effect of the number of lines inspected per hour compared to the number of major defects found. The trend line clearly shows that the more lines that are inspected in one hour, the less major defects are found. The data for this graph was taken from an actual inspection process in one of the projects I was involved in.
You should also try to spend a maximum of 4 hours on inspections as preparation for a meeting. That limits the number of lines for any inspection to roughly 800 lines.
When you take this into account, you should make sure that the team members are properly informed about which part of the code they will receive should be inspected. You must at all times make sure the inspection team all check the same piece of code. For example: When the code selected for inspection is 1500 lines long, then the inspection team will need to know where to start the inspection and where to stop.
Step 3 - Preparation
Roles involved
Description
During the preparation phase, the Inspectors and the Author inspect the code and log the defects they find in the defect log. Each defect should be categorized into one of the following two categories:
- Major defect. A major defect is any problem that will lead to problems when the code is used. This can be a problem that the end user will notice, or one that will cause another piece of code to misbehave. Major defects either change the program source code or would ultimately cause a source code change if not fixed in time.
- Minor defect. A minor defect is anything that doesn't fall into the Major defect category. Think of type errors, not conforming to the naming conventions, no comments in the code, etc.
The inspection team is allowed to use every tool available to them. The Inspectors may also contact the Author in case they need additional information. Neither the Author or the Inspectors are allowed to rework any of the problems. Code should be frozen until after the inspection meeting has taken place. I've had inspection meetings where during the meeting it became apparent that the Author already reworked some parts of the code. This resulted in a completely failed inspection meeting, because a number of the issues found where no longer there. But also, because new problems had been introduced.
Step 4 - Inspection meeting
Roles involved
- Moderator
- Reader
- Recorder
- Author
- Inspectors
Description
When all Inspectors, and the Author, have finished their inspection a meeting is called by the Moderator. The meeting is used to compare the findings and to create an inspection log. This inspection log is actually a accumulation of all defects on each separate defect log. Preferably, the meeting should be scheduled within a 2 or 3 days after the inspection team is invited to the meeting. This will make clear to all team members when their work should be finished.
The reader will lead the inspectors and authors through the code and check if and what kind of defects have been found. All defects will be logged in the Inspection log by the recorder. The Inspection log should also indicate which defects should be fixed during the rework phase.
Step 5 - Rework
Roles involved
Description
After the meeting, the author receives a copy of the inspection log and proceeds to fix the defects. It might happen that not all defects can be solved. If that is the case, then this should be reported back to the moderator. He/She will then need to decide what must be done with these defects.
Reworked code will need to be tested. Although this is not part of the inspection process, it should be clear that any changes to the code will need to be tested to make sure the changes do not introduce new problems in the application.
Step 6 - Follow up
Roles involved
Description
The Author informs the Moderator about the results of the rework he/she has executed. These results will be logged by the moderator and will be used for reporting purposes.
Step 7 - Report results
Roles involved
Description
The moderator now has all information available to update the reports. The data will be entered into the appropriate documents. These documents will be made available to all parties involved, including management. The reports will keep all parties informed on the progress of the inspection process.
Some last remarks
I have experienced problems trying to introduce this process into some of the projects I worked on. Biggest issue has always been trying to convince management to invest in this process. Because let's face it, this process takes time to implement and uses resources that most managers like to assign to actual coding tasks. It becomes increasingly difficult in projects where the deadline is approaching. Yet I found that in those situations, code inspections become even more vital in keeping the quality of the code on an acceptable level.
I said it at the beginning and I'll repeat it again here. This process is how I think it could be done. If you want to introduce a formal code inspection process in your development cycle, then there might be things you want to change. I do think though, that this document can be useful in setting up a formal code inspection process.
Resources
Previous blog posts by me on Code inspection:
Tools
Information on code inspections