Software Best Practices

We are struggling with developer resistance to performing peer (specifically code) reviews prior to a healthy dose of unit testing. The environment is on embedded avionics, so thorough testing requires a good deal of expensive and scarce equipment. I think that developers are more comfortable "showing their code in public" only after they're assured that it functions correctly. All the literature cites distinct advantages to total project cost when peer reviews are performed on code that compiles cleanly but prior to testing. I would be interested in others' experience with this sort of issue, including relevatn real world metrics that either bear out or contradict the existing literature.

Welcome to the Conversation!

I am going to ask a others to weigh in on this as well but I think you have a typical problem. Where I have helped role out inspections, I often meet initial resistance of people who don't want their code to be seen. Instead of trying to persuade them with facts, I try to hold a few "demo" sessions where we find one or two major issues. I then work with them to calculate how much time they saved to do other things than trying to debug it in live time.

The other thing that I can not emphasize enough is that you must have inspection focused on finding "issues" or, as one group called them, "saves". If it gets into solution creation or even the label "defects", it can turn off developers.

Increment code review throughout the software life cycle has repeatedly demonstrated high RIO.  The size of the code base, the expected number users, and the cost to defect, impact the RIO.  The higher cost, the more need for quality measures.

Selling management is part determining the RIO.  [There are books on the subject]

Sometimes, developers are embarrassed to show code to anyone, especially the so-called experienced developers.

Worst case, in places were I was unable to get developers to participate in increment code review, and unable to persuade managers of the RIO, I have conducted daily code reviews.  I pull the entire source code base, conduct a diff on the previous days full source base, and review the changes.  Then I politely talk to the individual developers about techniques to improve the code.

From the Java side, the first thing I establish on group projects is the Maven build process.  Maven provides plug-ins that helps with all incremental life-cycle development.

There are several issues here.

1) If developers are using modern unit test frameworks (which may not work in your embedded avionics environment, I know) then they are writing unit tests before they write code, so they can't do peer reviews before unit testing. That changes the order of things.

2) The most effective use of peer reviews is on requirements and designs. The ROI there is anything up to 150:1, i.e. it's up to 150 times cheaper in time to fix to do peer reviews up front on requirements (specifications) and designs. You don't say if your organization is doing peer reviews on the upstream artifacts or not

3) developers don't want their code reviewed before it compiles and would really like to test it first. However, that removes much of the benefit of doing the reviews. Doing peer reviews on code is about 3-4 time cheaper than testing to find errors.

As Earl says, if they see that an error they wouldn't have found is found in a peer review, they can become believers. The hard thing is to get them to do the peer reviews long enough so that they see the benefit to them - of errors that would have caused problems or caused them embarrassment if found later being found in reviews before the code is released.

I think it helps if a senior manager makes it clear that it's important to do the reviews. And then someone needs to track whether they are actually done, or not. And someone needs to show the benefits - the issues found per hour invested, for example. Or the reduction of defects found in system test. I was involved in implementing Inspections (a form of peer reviews) in an earlier company, and we used the reduction in errors the customers saw as a measure, as well as issues/hour.

That said, Construx doesn't necessarily review all the source code when we do a software project. We'll review all the requirements and designs. There is so much source code, it's hard to find the time to review it all. And the payback for reviewing the earlier stuff is so much greater.

You have a need for very highly reliable software, so for you reviewing all the code may make sense.

Also as Earl says, it's very important to make sure the comments in a review meeting are on the work product and not on the author. And to make the process as efficient as possible. For that, I suggest you investigate Inspections - they tend to find the largest percentage of errors and to be the most efficient process.

You might also investigate tools that can automate code reviews, for example Code Collaborator from Smart Bear Systems. If you go to their website (smartbearsoftware.com), they have a short book on getting programmers to do code reviews which you may find helpful even if you don't invest in a tool to help you. The book is called 'Best Kept Secrets of Peer Code Review', and although it's biased (it's written mainly by the developer of Code Collaborator) it does have some good ideas in it. Their tool allows programmers to comment online on the code at the line in question, and allows them to have a conversation about whether the code is wrong or not. They have some data on large companies using their tool to successfully do hundreds or thousands of code reviews.

Also, the data from the SEI on PSP and TSP projects shows that being very rigorous about peer reviews (they use Inspections) can make an enormous difference in quality in both system test and in production. See Noopur Davis and Julia Mullaney, 'The Team Software Process (TSP) in Practice: A Summary of Recent Results' SEI Technical Report CMU/SEI-2003-TR-014 available at the SEI website (www.cmu.sei.edu) http://www.sei.cmu.edu/pub/documents/03.reports/pdf/03tr014.pdf  - see page 38 for a summary of results.

Unit test are design artifacts, hence the unit test should be reviewed.

Some have argued, the unit test should always be reviewed, and even more strongly then any other element in the design and implementation process.  Unit test are a point of low-level design, as well as a point of confirmation of the interface protocol.  Re-usable, portability, scalability, etc. are heavily impacted at the unit level.

When conducted incrementally and daily, code reviews are low cost quality measures.

I’ve conducted daily reviews for a staff of 12 developers.  I dedicated 1 hour a every morning to the review task.  This was extremely helpful to people new to the staff and or technology.  Nipping issues in beginning with people new to the technology is extremely important.  I found daily reviews to be the most cost effective solution.

Empirical data shows that code reviews have a high pay off then unit test.  If unit test are being conducted, why not go the extra mile for the code reviews?

 

We're trying to improve our approach to reviewing at the moment, so I appreciate this thread.

I agree that reviewing the "inputs" is more valuable; so timing should perhaps be "as early as possible". A major aspect of timing is: how long does it take?

A useful source on this is Tom Gilb & son - http://www.gilb.com/community/tiki-list_file_gallery.php?galleryId=15 - they suggest: reject an artifact that breaks more than one strict rule "per page". This may take less than the 1-hour review suggested by SmartBearSoftware, of course, if you find three or four rules broken on the first page.

A couple of strict Gilb rules (for  statements of requirements) are: "must be possible to test"; and "must be unambiguous". I'm proposing some strict rules for the technical side of things, here, including (for some of our naming conventions): "must be consistent with our version control organization"; "must be consistent with our deployment automation".

What do people think about coverage? I'm proposing: individuals to review small samples of 100% of the documents and code, as above, instead of reviewing a small number of things thoroughly, in a group. (Your domain may be stricter :-) The idea is to invest our limited time for the best feedback about the quality of what we (are about to) deliver.

I'll try to post about the emotional side of this later, if I feel up to it...

regards, Bob Corrick (Developer)

The concept of reviewing for rules validation bothers me.  In the Java arena, there is numerous free and open source software (FOSS) offerings that can do automated rules review.  The rules can be anything from the basic to complex rules validation.

Normally, I just have everybody turn on all compiler warnings.  The compiler warnings catch everything from unused variables to null pointers, bad naming conventions, and much more.  [The IDE I use even goes as far as pointing out misspellings in the code comments and documentation.]

Tools like findbugs (FOSS) take the compiler warnings to another level by allowing developers to write their own rules.  Findbugs is easily integrated into the build processes such as Ant and Maven.

Hence, the concept of code review to find ‘rules validation’ is old school.

I think code review should be utilized to find misuses of the technology, bad programing, etc.  For instances, many developers new to Java do NOT realize that there is a String Tokenizer in the Java API, and code their own.  The Tokenizer is something that shows up repeatedly.  Other common problem include bad interface design, limited or no unit testing, cloned code, etc. 

NOTE: All the problems I just mentioned are vastly disappearing.  There are FOSS tools that determine the quality of design, the level of unit testing coverage, and search for cloned code.  Each of the tools can be integrated into the daily build process.

Hence, the job is moving more from ‘code review’ to constructing a proper build process.

BTW: I realize some of this is new to people in the Microsoft world.  Microsoft has just caught on to some of the techniques.  Microsoft offering cost a small fortune, hence limiting the use of the techniques.

Bob,

I like the idea of a few strict rules if they are the rules that, if broken, will cause you the most pain and suffering. I don't like strict rules for their own sake.

I also like the idea of investing the limited time again where a mistake will cause the most pain. These are things you may even want to do the live peer review practice of collaborative construction. The trick here is, I guess, that you have to do some level of risk assessment to find out where your pain points are.

 Jumping into the middle of an already-going conversation can be a bit dangerous, but...

First off, I think you need to be careful about what you mean by "review". Reviews come in all shapes and sizes and for all kinds of different purposes. Sometimes a "design review" to kick around some current ideas is the right thing. On the other hand, something like "inspections" has a very specific purpose--find defects. So depending on what you want your review to accomplish, the "correct" time could vary significantly.

Second, as many of you may already know I'm a big fan of inspections (with the goal of finding as many defects as possible). That said, it makes no sense to inspect something before the authors are confident they are done with it. But that's a different issue from an earlier post--how to get the developers to be motivated to call something done.

Third, while the vast majority of Inspections literature is on code inspections my view is, "Code inspections are better than nothing, but not by much". We've gone into several projects with the intent of inspecting everything, requirements, design, code, test cases, ... The bottom line is that in all of these cases we abandoned code inspections well before half way into the project. The problem was that well done requirements inspections and design inspections left so few defects in code that it wasn't worth inspecting it. The code inspections ended up being an expensive way to enforce the coding standard. Malcolm (Hi, Malcolm--good to hear from you, it's been a while) is right, there are tools that can do alot of that for you so why waste people's time on it.

 -- steve

Our code review practices have been very effective in reducing the number of defects delivered from development to the test group.  First, understand that this is within a framework where the mantra for the development team is "zero defects delivered to QA" .

Our practice is to review code at the point where the developer will declare it 100% ready to be delivered.  So all unit testing is complete, all source code comments are in place, all formatting is done...100% complete. 

This is a little different philosophy than some of other posts here, but it's working well for us and has for a long time.  Years ago I did reviews earlier, prior to 100% complete but value was diluted because the discovery of issues was not seen as a notable event, since the developer had not declared the code complete.  With our method, discovery of an issue is universally recognized as a real "save" because the code had been declared ready to go to QA, and the issue would have gotten to QA if we had not done the review.  Thus the value of the review is widely acknowledged even among the initially resistant.

For bug fixes, the review is triggered by the point in our workflow-based defect tracking system where the developer declares the code complete, and is usually done by one senior developer.  For larger units of new functionality we put code reviews on the project plan and do a group review with code projected up on the wall.

As for reviewing for coding rules vs. logic errors: When we start reviews within a group that has never done them before, we do focus on comprehensive coding rule compliance, and yes it annoys people.  But after a few reviews when everyone realizes they really are going to get the code returned to them for coding rule violations, the problem takes care of itself.  So in practice our reviews are always about logic and design realization since the coding rules are universally followed after the first month or so.  And we don't have a huge book of coding rules, just a minimal set of key conventions -- variable naming, error handling, use of globals and statics, that sort of thing.

By 100% complete I meant that the developer is 100% complete with coding, commenting, and unit testing (and yes formatting, but I don't tend to think of that as a separate step) .  In other words, the developer asserts that the code does not need to be touched again for any reason unless the code review reveals an issue.

100% does not mean it has been delivered to QA.  In our process, code reviews are done prior to integration testing, which is done prior to delivery to QA.

Is the code considered 100% complete when the code is checked into the source repository, or when the developer marks the code complete through some other means such as an issue tracking system, repository tag, or something else?

The answer to that varies a little bit from one team to another, but in general, they make an explicit action to declare the code complete, and that action is not triggered by the source code management system (for most, it's part of the issue tracking system).  Different teams have different conventions for checking in code, for automated vs. manual builds, and other practices that are adjacent to the code review practice.  But on all teams, the developers explicitly declare when the code is 100% complete and ready for review.

100% complete says: "I am ready for review"?

 Several issues with this method:

• In my opinion, only valid versions of the code should be checked into the source repository. Since the source repository only holds valid version of the code, the code is 100% complete.
• If 100% complete state is required prior to reviews, then there is missed opportunity for fixing problems early.

 I am not comfortable with the concept of "100% complete" as described.

• What is the purpose of review after 100% complete?
• Is the focus "zero defects delivered to QA" as mentioned earlier?
• Is 100% complete a mantra rather than practice?

I think the real issue in doing peer reviews is not so much in the timing of when you do them but more in what they are intended to accomplish.

On the projects I ran peer reviews were intended to accomplish three things:

1.       Identify and correct as many bugs as possible. 

2.       Ensure that the code was well structured and maintainable.  The focus was on ensuring that each program was documented and structured in such a way that someone other than the original programmer could maintain a program once it went into production.

3.       Allow developers to learn from each other.  This was an opportunity for more senior programmers to mentor less experienced programmers on more efficient or better ways of coding and structuring programs.      

If you delay the peer reviews until the program is completely coded and tested there is little chance the programmer is going to want to spend time changing comments or making the program more maintainable.  You also lose out on the benefits you derive from more experienced programmers mentoring less experienced programmer. 

Inspection of programs after they are completely coded will identify and eliminate defects but once the program is completely coded and tested you probably have little chance affecting program maintainability or the efficiency of the code.

I'm not sure why the formatting of my previous reply go so messed up.  Is there anyway I can edit it, so I can fix the formatting problems and make it easier to read?

SixSigmaGuy,

You can try copying the message to Word and reformat it so it is more readable.

Then you can copy the formatted text from Word and paste it in a new message.  I've done this a couple of times and it looks like the Word formatting is retained when you paste the text.

I wrote the message in Word and pasted it there.  It looked fine until I clicked the post button.  I always write my posts in Word first to take advantage of the spelling and grammar tools, but this time it didn't work for me.  I'll try it again though.