You would think that there are three required items in a code review: A checklist of things to look for, code to review, and someone to review it. This assumes that the code review follows the rote mechanics described in the previous section.
We’ve taken a slightly different tack than many firms; we’ve determined there are actually two parts to a code review. The first part is a somewhat mechanical inspection that can be performed by a technical support level person, instead of a developer or analyst.
Our QA department has the responsibility of doing this first inspection. They use a checklist to inspect the code and then report back to the developer. It is ultimately up to the developer to make the changes, but the second rt of the review process will catch the developer if they try to slide by.
Once QA has been through the code, it’s time for a group review. Does every piece of code get sent through code review? Uh, yeah, sure it does. Of course! Why wouldn’t it? Well, we’re human. We make impossible schedules, commit to deadlines that are silly, and once in a while, people get sick or forget. But we do track which pieces of an application have been through a code review and which haven’t. At some point in the future, we’ll have enough data to do some meaningful analysis - I’m terribly curious about the effectiveness of code reviews - do the modules that have had code reviews reflect the effort in terms of quality? In other words, have our code reviews reduced bug counts?
Now it’s time for a developer to Defend Their Life. While time consuming, the group review is a lot more valuable than a one-on-one. I’ve covered several of the reasons, but there’s one more. Even the most senior developer (uh, I guess that would be me) is subject to a code review. Having the others in the shop ask me why I didn’t delete a temp file at the end of a routine is good for their egos (hey, even he screws up sometimes!) and keeps me honest.
It is important to make sure that the code review is a building process, a process of constructive criticism. While I’ve joked about our group „Defending Your Life“ reviews, it’s still a positive experience - not an event where we’re all going to rip on somebody, make fun of them and make their life miserable. If that’s the type of attitude you have, then no one will participate in them.
Of course, we have certain expectations, and if someone came in unprepared and made a poor presentation, then they would get what they deserve.
The purpose of a code review is to make sure a developer follows the rules. This begs the question: What are the rules? Obviously, a worthwhile code review assumes a set of pre-defined guidelines that the developers should be following. It’s pretty hard for a developer to write good code if there has been no definition of what „good“ is.
Does this mean that you have spend hours, days, and months to put together a set of guidelines before you do your first code review? It depends. If you don’t have any documented company standards, it probably means that you’ve got a number of cowboys who each do things their own separate way. You’re going to have to spend some time to get them to agree on at least a minimal set of commonly accepted rules.
But you don’t have to stop the presses for a year in order to do so. If your company guidelines consist of a half page of notes taken from a seminar someone attended three years ago, here’s a trick that will get your guidelines up to industrial strength in a surprisingly short time.
You can gradually build a set of guidelines by having one person document „good ideas“ as you go through your code reviews. (Yes, you still have to do your code reviews! You’re not getting off that easily!) We started with a fairly detailed set of standards and guidelines, but we still add items to the list as we go through group reviews. This is an excellent way to build or enhance a code review checklist, because it achieves buy-in while it’s being put together.
Your checklist does not have to be a 90 page manual, and, in fact, probably shouldn’t be. If it is, no one will pay attention to it - who could remember all of it? And how in the world would you bring a new developer up to speed?
One more thing about the guidelines - the term is guidelines, not absolute rules that must be followed or we’ll cut off your hand. There are always situations where a standard doesn’t make sense. Forcing people to adhere to the standard mindlessly produces code no better than had there been no guidelines in the first place.
A lot of you are hankering for an example of a code review checklist, and here I fear I will disappoint. One of the goals of this book is to be language independent, but by its very nature, a code review is language specific. Nonetheless, there are some general practices that do not vary from language to language, or that can be adapted to handle the specific syntax and nuances of a tool. Let’s look at an outline of a code review that you can use as a starting point.
The first part of the review is done by a technical support type of person. The first group of things to check don’t even involve code, but I’ve seen each one of these violated in an application that someone has asked me to fix.
Getting into the code, here are some of the things that can be checked by a non-developer:
The next group of things to check can be checked by a highly skilled technical support level person, or by an entry level developer:
Finally, we get to the items that generally need the expertise of a developer to look at:
Obviously you’ll be able to come up with additional items for your checklist that are a function of your chosen language and your particular development style.
A test data set is critical for accurate testing. Test data sets vary according to the type of application, but generally, they should contain a variety of records that appropriately describe the variations likely in the user’s data.
Pay particular attention to boundary conditions, field sizes, and data types. Another thing to consider is the eventual size of the system’s data set. It is possible that an application performs acceptably with 100 records but will bog down significantly once the live data set of 50,000 records is loaded.
A useful feature both for testing and training is that of Proof Cases – a data set that demonstrates a specific feature or function. For example, in a Quoting system, one Quote in the test data may demonstrate a Quote with a single item, a second Quote in the test data would demonstrate a Quote with a multiple custom items.
Often you’ll need multiple data sets – in order to be able to roll back to a certain point in time. This is particularly important for processes.
Having multiple data sets that mirror conditions at various times through a processing cycle can be invaluable to quickly pinpoint problems. Nothing is more frustrating than having to wait 15 minutes for a posting process to run just to get to the point of failure.
One of the most common reasons that a routine works at 11:46 but not at 11:47 is that the data that was entered in the last test is causing problems. Either it was bad, it got corrupted, it was incomplete, or it wiped out something else that shouldn’t have been wiped out.
One of the two fundamental rules we’ve had was to create a clean set of original test data so that you have an accurate and measured baseline from which to start. Each time you are ready to run a test, you can copy all the data from the original test data directory into the current test data directory and run off that new set of test data.
The cardinal rule for original test data: Make it clean, robust and keep it in a location that can be accessed easily so that the test data directory can be replenished with this clean data on demand.
Unit testing involves the actual examination of every screen, process and report in a system on a granular basis.
Yes, this sounds really stupid, but how many times have you gone into a customer and said, „Oh and by the way, if you push this, oh no! I thought that was working!“ We’ve all done it. This is QA’s first responsibility - is to make sure that every one of those buttons does something. Sometimes we even forget to put a button in on one screen but it’s on all the other screens. So they push all the buttons and make sure everything is functioning.
QA creates a sheet that lists every object on a screen, and then checks off each object as it’s tested. All the buttons are pushed, all the checkboxes are checked, all the fields have data entered. Tab order is another item that is checked in this phase - when you tab through the screen, does the focus shift appropriately - and, do all the keyboard shortcuts work?
When you hit add, enter data into every field, and press Save, is the record actually added? Does all the data get added? Did the entire entry in the field get saved, or did we chop off the last five characters in the invoice number? This is why QA has to be comfortable with FoxPro - they need be able to verify what is happening in the application down to the raw table level.
There are three general types of rules in an application - those attached to a field, those attached to a form, and application-wide rules. In all three cases, these rules have to be identified and then we have to test for them. The description of how we are going to do so is our test plan, and it’s contained in the functional spec (or the change order.)
The next step for QA is to execute the test plan. It is up to the developer to make sure that the test plan is robust, but it is also up to the QA person to figure out ways around it and figure out ways to break it.
The developer should be able to put together a case chart to handle all of the possible options within a program and provide test data or require a test plan to handle these cases. As QA tests each option, they should be able to check off which options were tested and what the result was. For example:
As developers, we know our application works. Right? We don’t need anybody to prove to us that it works - we’ve run the application after writing the code and we’ve seen that it works. Of course, we’ve only seen that it works one time - why would we waste our time trying it more than once?
The mission of QA is not to prove that the application will work - their mission is to break the application. And note that testing is a different type of activity than any other part of the software development process - because the testing is never done. A thousand hours of testing can go into an application, and the only thing that you can be sure of is that you haven’t found a bug - yet.
So the final step in the testing process is to pull out all the stops and to be devious, conniving, downright evil in trying to find ways to break the applications. The greatest fear of a testing person is that there’s a bug in the system that they didn’t find.