Code reviews?

Be sure that the tests do test all that should be tested, if nothing else get someone else to review the test vectors/conditions and the RESULTS. However module testing can sometimes go awry if external events screw things up, if no locking fo resources are performed and something else tampers with part of it (e.g. DMA buffers), the module may work on its own but not when integrated.

Some of the code I have written has been near impossible to create the millions of variations possible in source data (video frames), only partial cases and often it is easier to force conditionsto extremes of noise and characteristics of capture method. Creating test data sets to test the code usually creates data sets that are too 'clean' to cover all areas of problems.

Too true. Where possible when working on my own I often at least talk through the algorithm flow to ensure what can be done with someone who may understand the maths or purpose enough to highlight potential problems. This can often be useful as a debugging aid for conditions that upset part of an algorithm. The algorithm may well be split across MANY modules.

Unit testing works on some parts, but falls down in my experience with asynchronisity of various levels of integration. Especially with external events, from the mundane of operator actions to the variabilities of video sync delay timing (or dropout of said same for some sources).

drain.

Nice aneckdote ;) I wonder if extensive code reviews leads more often to the false assumption that everything is covered for.

Nice rule and nice anecdote to go with it.

Each review has to start with a number of cross-checking actions.

Specification issue level and date; Code issue and date (perhaps even time); What the Configuration Management Record believes to be the current status. Problem Reports Received and the records. Latest Change Proposals and Work Instruction Record.

When all (the flowchart on my HIDECS page gives you an idea of my own process) those items check out with each other correctly then you can begin to look at the item to be reviewed.

As for the number of reviews:-

I prefer to resolve the specification first (usually up to 3 iterations) before I progress to starting the coding. When I do start the coding the first run through is normally just writing the glossary comment - a sort of mini-requirement statement - so that I know what I have planned for each module I am going to build. These glossary comments are then reviewed, before the code is written, to ensure that the intent for each module is in conformity to the spec. This might take two iterations at most. When I start with real code it is tested for intended function by the programmer almost immediately it is written (remember I use Forth for the most part and Forth does encourage small functional elements at whatever abstraction level you are dealing with). There follows a further review which incorporates a full Fagan style inspection. If this is passed the code is issued for testing where a formal functional test and a limits test are performed. The results come back to the review panel who then review the test results and, if all is OK, approve the code for release to the trusted library of functions ready to build the rest of the system.

So, from requirements to system integration there would have been 9 or 10 reviews of each element. Once integrated there would be further reviews of the unit testing, system testing and final integrations testing. As a wrap up there may also be a formal client witnessed factory acceptance test of the whole system which would involve yet another review.

While it may seem that there are a lot of reviews the time taken to accomplish them is of the order of 8% to 10% of the project. The benefit is that it is quite possible to eliminate at least 95% of all the bugs you are ever likely to see in the project by taking the issue of proper review and testing seriously.

It is interesting to note that the longest review meetings actually happen very early in the project when the requirements and specification documents are being resolved. These review meetings have occassionally broken the two hour rule but at that stage it was always worthwhile geting the issues sorted.

Many lines of code. Not so many programmers. If you want to review all the code it has to be divided into many reviews. For example if you have 15 programmers that produces 500.000 lines of code in one and a half year. How many reviews is that?

Not bad but expensive (which actually is bad). The feeling I have is that I get better pay-back from static analyzers and unit testing.

So, half a million LOC in 18 moths and no code review? I am certain that you may have found out quite a bit more about the code by just doing a static walk-through a sub-routine at a time.

I have found that the review process I go through, in my average project, will actually eliminate code lines. I may write more code than ends up in the project but only that which meets the needs of the requirements will remain, useless flourishes will be evicted and the code gets much tighter and resilient through the review process. I doubt that tool like LINT or other static checking tool will have that sort of effect to any great degree.

No, I didn't say anything about the number of code reviews we had. And I´m not saying that code reviews are bad and that you shouldn´t have them. Every programmer had about 2-3 reviews of a rather small part of the code they had produced. I think I would like to have had twice that number of reviews. The focus of the reviews was mainly to check that our coding standard and rules and recommendations was followed - a way to get everybody on the same track. The rules and recommendations describes how the software should be developed on a level above the coding standard. It actually handles many of the problems that would have been discussed on a code review.

Write a theory of operations of the code/device. As you do, you will review and revisit the code to refresh your memory as to why you did what you did.

Sincerely,

MarcW.

mart> A bit of googling shows that code reviews are a "good idea"