Hiển thị các bài đăng có nhãn peer reviews. Hiển thị tất cả bài đăng
Hiển thị các bài đăng có nhãn peer reviews. Hiển thị tất cả bài đăng

Embedded Code Quality and Best Practices Training Videos full length

I've posted the full series of my available embedded system code quality and related best practices videos on YouTube.  These are full-length narrated slides of the core set of safety topics from my new course.  They concentrate on getting the big picture about code quality and good programming practices.
Each of the videos is posted to YouTube as a playlist, with each video covering a slide or two. The full lecture consists of playing the entire play list, with most lectures being 5-7 videos in sequence. (The slide download has been updated for my CMU grad course, so in general has a little more material than the original video. They'll get synchronized eventually, but for now this is what I have.)

Obviously there is more to code quality and safety than just these topics. Additional topics are available slides-only.  You can see the full set of course slides including for those lectures and others here:
  https://users.ece.cmu.edu/~koopman/lectures/index.html#642

New Peer Review Checklist for Embedded C Code

Here's a new peer review checklist to help improve the quality of your embedded C code.

To use the checklist, you should do a sit-down meeting with, ideally, three reviewers not including the code author. Divide the checklist up into three portions as indicated.  Be sure to run decent static analysis before the review to safe reviewer time -- let the tools find the easy stuff before spending human time on the review.

After an initial orientation to what the code is supposed to do and relevant background, the review process is:
  1. The review leader picks the next few lines of code to be reviewed and makes sure everyone is ONLY focused on those few lines.  Usually this is 5-10 lines encompassing a conditional structure, a basic block, or other generally unified small chunk within the code.
  2. Reviewers identify any code problems relevant to their part of the checklist.  It's OK if they notice others, but they should focus on individually considering each item in their part of the checklist and ask "do I see a violation of this item" in just the small chunk of code being considered.
  3. Reviewer comments should be recorded in the form: "Line X seems to violate Checklist Item Y for the following reason." Do NOT suggest a fix -- just record the issue.
  4. When all comments have been recorded, go back to step 1.  Continue to review up to a maximum of 2 hours. You should be covering about 100-200 lines of code per hour. Too fast and too slow are both a problem.
A text version of the checklist is below. You can also download an acrobat version here.  Additional pointers to support materials are after the checklist. If you have a static analysis tool that automates any of the checklist item, feel free to replace that item with something else that's important to you.

===============================================================
Peer Review Checklist: Embedded C Code
       
Before Review:
0    _____    Code compiles clean with extensive warning checks (e.g. MISRA C rules)
       
Reviewer #1:       
1    _____    Commenting:  top of file, start of function, code that needs an explanation
2    _____    Style is consistent and follows style guidelines
3    _____    Proper modularity, module size, use of .h files and #includes
4    _____    No orphans (redundant, dead, commented out, unused code & variables)
5    _____    Conditional expressions evaluate to a boolean value; no assignments
6    _____    Parentheses used to avoid operator precedence confusion
7    _____    All switch statements have a default clause; preferably an error trap
       
Reviewer #2:       
8    _____    Single point of exit from each function
9    _____    Loop entry and exit conditions correct; minimum continue/break complexity
10    _____    Conditionals should be minimally nested (generally only one or two deep)
11    _____    All functions can be unit tested; SCC or SF complexity less than 10 to 15
12    _____    Use const and inline instead of #define; minimize conditional compilation
13    _____    Avoid use of magic numbers (constant values embedded in code)
14    _____    Use strong typing (includes: sized types, structs for coupled data, const)
15    _____    Variables have well chosen names and are initialized at definition
       
Reviewer #3:       
16    _____    Minimum scope for all functions and variables; essentially no globals
17    _____    Concurrency issues? (locking, volatile keyword, minimize blocking time)
18    _____    Input parameter checking is done (style, completeness)
19    _____    Error handling for function returns is appropriate
20    _____    Null pointers, division by zero, null strings, boundary conditions handled
21    _____    Floating point use is OK (equality, NaN, INF, roundoff); use of fixed point
22    _____    Buffer overflow safety (bound checking, avoid unsafe string operations)
       
All Reviewers:     
23    _____    Does the code match the detailed design (correct functionality)?
24    _____    Is the code as simple, obvious, and easy to review as possible?
       
        For TWO Reviewers assign items:   Reviewer#1:  1-11; 23-24    Reviewer#2: 12-24
        Items that are covered with static analysis can be removed from checklist
        Template 1/28/2018:  Copyright CC BY 4.0, 2018, Philip Koopman
===============================================================

Additional material to help you with successful peer reviews:


Guest on Embedded.fm Podcast

Elecia & Chris invited me to chat with them on this week's Embedded.fm podcast and it was a lot of fun.

You can check out my episode here:

http://embedded.fm/episodes/183

Also, I highly recommend listening to Jack Ganssle's excellent episode 53: "Being a grownup engineer"

http://embedded.fm/episodes/53

Scroll through the episode list.  I'm episode 183 so you can tell they've been at this quite a while. There's a lot of great stuff to listen to.

Note added Tue. 1/17:  books are back in stock in Amazon.

Meanwhile, if you are ordering from the US, the best deal on the book is via paypal here: http://koopman.us/

Peer Review Tutorial

Here's a preview video on Peer Review (about 1 minute of content, plus leader/trailer stuff).  There is also a full version of this video available for free from the Edge Case Research video library (see below for details).

Notes:
  • Peer review finds half the defects for 10% of the project cost. (Blog)
  • But they only work if you actually do the reviews! (Blog)
  • If you're finding more than 50% of your defects in test instead of peer review, then your peer reviews are broken. It's as simple as that. (Blog)
  • Here's a peer review checklist to get you started. (Blog) You should customize it to your needs.


Full tutorial video: https://vimeo.com/181433327

For more about Edge Case Research and how to subscribe to our video training channel, please see this Blog posting.






Boehm's Top 10 Software Defect Reduction list

A while back, Barry Boehm & Vic Basili wrote a nice summary of best ways to get better quality software. Their advice still applies to embedded systems today. Below is their list (in bold) with my commentary (the parts not in bold).

1. Finding and fixing a software problem after delivery is often 100 times more expensive than finding and fixing it during the requirements and design phase

Bug fix cost gets worse as your software gets closer to deployment, because you have to not only spend a lot of time tracking down the source of the bug, but also retest the system after the fix.  It is common to find situations in which a bug "escape" into field units costs dramatically more than 100x.  Consider having to recall a fleet of cars to install a bug fix, or do maintenance visits to thousands of sites to manually install a fix.  (Over-the-air fixes have their own problems, but that's a topic for another time.)

2. Current software projects spend about 40 to 50 percent of their effort on avoidable rework.

As the joke goes, the first 90% of the project is spent on design, and the second 90% on debugging.  The cheapest way to debug is to avoid putting the bugs into the software in the first place.  The next best way is to find them at the point of introduction (e.g., peer review of design before code is written) rather than at system test.

3. About 80 percent of avoidable rework comes from 20 percent of the defects

If you have a "bug farm" that's often not because the code is bad, but rather because the underlying requirements and design are bad.  If one module has a lot of bugs you should rewrite the module rather than keep patching it.  However, if during the rewrite you might well discover that the problem isn't really the code, but rather a bad design or unclear requirements. Writing new code for a bad design ultimately won't solve the problem.

4. About 80 percent of the defects come from 20 percent of the modules, and about half the modules are defect free. 

In addition to comments for #3 above, modules with high cyclomatic complexity tend to be difficult to test and tend to be more bug-prone.  Keeping a limit on complexity can help with this problem.

5. About 90 percent of the downtime comes from, at most, 10 percent of the defects.

It makes sense to weight testing on the areas that are the highest risk.  For desktop software this often corresponds to the common use cases.  For embedded systems and other mission-critical systems this also means testing failure detection and recovery for high-cost failures.

6. Peer reviews catch 60 percent of the defects.

Yes, really.  Peer reviews catch the majority of defects.  Why aren't you doing them?  (If you are doing them, are they catching at least half the defects?)


7. Perspective-based reviews catch 35 percent more defects than nondirected reviews.

When you do reviews, give each reviewer a checklist with a different set of areas to think about or different role to play.  For example, control flow, data flow, exception handling, testability, coding style.

8. Disciplined personal practices can reduce defect introduction rates by up to 75 percent.

As much fun as it is to be a coding cowboy, on average even the best programmer will introduce fewer bugs by following a methodical engineering practice rather than slinging code. As mentioned above, the cheapest bugs to fix are the ones that never made it into the code.  Beyond this, there are practices such as PSP and TSP that are shown to dramatically improve quality without really changing productivity.

9. All other things being equal, it costs 50 percent more per source instruction to develop high-dependability software products than to develop low-dependability software products. However, the investment is more than worth it if the project involves significant operations and maintenance costs.

In other words, if a product recall puts your company out of business, it's worth investing in good software quality up front.  In my experience if you are already shipping a mission-critical product you're already spending that 50 percent more (and then some), but still shipping defects.  This isn't saying spend even more.  Rather, doing peer reviews and some other basic software quality practices you can improve quality at the same cost you're already spending.  Testing software into submission is simply not the way to go.

10. About 40 to 50 percent of user programs contain nontrivial defects.

If you have programmable features, your customers will have bugs in what they do.  And don't forget that your financial management spreadsheets are user programs (i.e., can, and often do have bugs).

Items #1 - #10 from:
  Boehm & Basili, Software Defect Reduction Top 10 List, IEEE Computer, Jan. 2001, pp. 135-137.

You can read the original three-page article here:
  https://www.cs.umd.edu/projects/SoftEng/ESEG/papers/82.78.pdf

Top 5 Embedded Software Problem Areas

The biggest problems I see in industry code reviews are code complexity, real time performance, code quality, weak development process, and dependability gaps. Here's an index into blog postings and other sources that explains the problems and how to deal with them.

-----------------------------

Several times a year I fly or drive (or webex) to visit an embedded system design team and give them some feedback on their embedded software. I've done this perhaps 175 times so far (and counting). Every project is different and I ask different questions every time. But the following are the top five areas I've found that need attention in the past few years.  "(Blog)" pointers will send you to my previous blog postings on these topics.

(Don't miss our growing video tutorial library that covers some of these issues.)

How does your project look through the lens of these questions?

(1) How is your code complexity?
  • Is all your code in a single .c file (single huge main.c)?
    • If so, you should break it up into more bite-sized .c and .h files (Blog)
  • Do you have subroutines more than a printed page or so long (more than 50-100 lines of code)?
    • If so, you should improve modularity. (Blog)
  • Do you have "if" statements nested more than 2 or 3 deep?
    • If so, in embedded systems much of the time you should be using a state machine design approach instead of a flow chart (or no chart) design approach. (Book Chapter 13). Or perhaps you need to untangle your exception handling. (Blog)
    • If you have very high cyclomatic complexity you're pretty much guaranteed to have bugs that you won't find in unit test nor peer review. (Blog)
  • Did you follow an appropriate style guideline and use a static analysis tool?
    • Your code should compile with zero warnings for an appropriate warning set. Consider using the MISRA C rule set (Blog) and a good static analysis tool. (Blog)
  • Do you limit variable scope aggressively, or is your code full of global variables?
    • You should have essentially zero global variables. (Blog)  
    • It's not that hard to get rid of globals if you look at things the right way. (Blog)

(2) How do you know your real time code will meet its deadlines?
  • Did you set up your watchdog timer properly to detect timing problems and task death?
    • The watchdog has to detect the death or hang of each and every task in the system to provide a reasonable level of protection. (Blog)
    • How long to set the watchdog is a little trickier than you might think. (Blog)
  • Do you know the worst case execution time and deadline for all your time-sensitive tasks?
    • Just because the system works sometimes in testing doesn't mean it will work all the time in the field, whether the failure is due to a timing fault or other problems. (Blog)
  • Did you do scheduling math for your system, such as main loop scheduling?
    • You need to actually do the scheduling analysis. (Blog ; Blog)
    • Less than 100% CPU usage does not mean you'll meet deadlines unless you can verify you meet some special conditions, and probably you don't meet those conditions if you didn't know what they were. (Blog) 
  • Did you consider worst case blocking time (interrupts disabled and/or longest non-context-switched task situation)?
    • If you have one long-running task that ties up the CPU only once per day, then you'll miss deadlines when it runs once per day.  But perhaps you get lucky on timing most days and don't notice this in testing. (Blog)
    • Did you follow good practices for interrupts?
      • Interrupts should be short -- really short. (Blog) So short you aren't tempted to re-enable interrupts in them. (Blog)
      • There are rules for good interrupts -- follow them! (Blog)
    (3) How do you know your software quality is good enough?
    • What's your unit test coverage?
      • If you haven't exercised, say, 95% of your code in unit test, you're waiting to find those bugs until later, when it's more expensive to find them. (Blog) (There is an assumption that the remaining 5% are exception cases that should "never" happen, but it's even better to exercise them too.)
      • In general, you should have coverage metrics and traceability for all your testing to make sure you are actually getting what you want out of testing. (Blog)
    • What's your peer review coverage?
      • Peer review finds half the defects for 10% of the project cost. (Blog)  But only if you do the reviews! (Blog)
    • Are your peer reviews finding at least 50% of your defects?
      • If you're finding more than 50% of your defects in test instead of peer review, then your peer reviews are broken. It's as simple as that. (Blog)
      • Here's a peer review checklist to get you started. (Blog)
    • Does your testing include software specific aspects?
      • A product-level test plan is pretty much sure to miss some potentially big software bugs that will come back to bite you in the field. You need a software-specific test plan as well. (Blog)
    • How do you know you are actually following essential design practices, such as protecting shared variables to avoid concurrency bugs?
      • Are you actually checking your code against style guidelines using peer review and static analysis tools? (Blog)
      • Do your style guidelines include not just cosmetics, but also technical practices such as disabling task switching or using a mutex when accessing a shared variable? (Blog) Or avoiding stack overflow? (Blog)

    (4) Is your software process methodical and rigorous enough?
    • Do you have a picture showing the steps in your software and problem fix process?
      • If it's just in your head then probably every developer has a different mental picture and you're not all following the same process. (Book Chapter 2)
    • Are there gaps in the process that are causing you pain or leading to problems?
      • Very often technical defects trace back to cutting corners in the development process or skipping review/test steps.
      • Skipping peer reviews and unit test in the hopes that product testing catches all the problems is a dangerous game. In the end cutting corners on development process takes at least as long and tends to ship with higher defect rates.
    • Are you doing a real usability analysis instead of just having your engineers wing it?
      • Engineers are a poor proxy for users. Take human usability seriously. (Blog; Book Chapter 15)
    • Do you have configuration management, version control, bug tracking, and other basic software development practices in place?
      • You'd think we would not have to ask. But we find that we do.
      • Do you prioritize bugs based on value to project rather than severity of symptoms? (Blog)
    • Is your test to development effort ratio appropriate? Usually you should have twice as many hours on test+reviews than creating the design and implementation
      • Time and again when we poll companies doing a reasonable job on embedded software of decent quality we find the following ratios.  One tester for every developer (1:1 head count ratio).  Two test/review hours (including unit test and peer review) for every development hour (2:1 effort ratio). The companies that go light on test/review usually pay for it with poor code quality. (Blog)
    • Do you have the right amount of paperwork (neither too heavy nor too light)
      • Yes, you need to have some paper even if you're doing Agile. (Blog) It's like having ballast in a ship. Too little and you capsize. Too much and you sink. (Probably you have too little unless you work on military/aerospace projects.)  And you need the right paper, not just paper for paper's sake.  (Book Chapters 3-4)

    (5) What about dependability aspects?
    • Have you considered maintenance issues, such as patch deployment?
      • If your product is not disposable, what happens when you need to update the firmware?
    • Have you done stress testing and other evaluation of robustness?
      • If you sell a lot of units they will see things in the field you never imagined and will (you hope) run without rebooting for years in many cases. What's your plan for testing that? (Blog)
      • Don't forget specialty issues such as EEPROM wearout (Blog), time/date management (Blog), and error detection code selection (Blog).
    • Do your requirements and design address safety and security?
      • Maybe safety (Blog) and security (Blog) don't matter for you, but that's increasingly unlikely. (Blog)
      • Probably if this is the first time you've dealt with safety and security you should either consult an internal expert or get external help. Some critical aspects for safety and security take some experience to understand and get right, such as avoiding security pittfalls (Blog) and eliminating single points of failure. (Blog)
      • And while we're at it, you do have written, complete, and measurable requirements for everything, don't you?  (Book Chapters 5-9)
    For another take on these types of issues, see my presentation on Top 43 embedded software risk areas (Blog). There is more to shipping a great embedded system than answering all the above questions. (Blog) And I'm sure everyone has their own list of things they like to look for that can be added. But, if you struggle with the above topics, then getting everything else right isn't going to be enough.

    Finally, the point of my book is to explain how to detect and resolve the most common issues I see in design reviews. You can find more details in the book on most of these topics beyond the blog postings. But the above list and links to many of the blog postings I've made since releasing the book should get you started.



    Not Getting Software Wrong


    The majority of time creating software is typically spent making sure that you got the software right (or if it's not, it should be!).  But, sometimes, this focus on making software right gets in the way of actually ensuring the software works.  Instead, in many cases it's more important to make sure that software is not wrong.  This may seem like just a bit of word play, but I've come to believe that it reflects a fundamental difference in how one views software quality.

    Consider the following code concept for stopping four wheel motors on a robot:

    // Turn off all wheel motors
      for (uint_t motor = 1; motor < maxWheels; motor++)  {
        SetSpeed(motor, OFF);
      }

    Now consider this code:
    // Turn off all wheel motors
      SetSpeed(leftFront,  OFF);
      SetSpeed(rightFront, OFF);
      SetSpeed(leftRear,   OFF);
      SetSpeed(rightRear,  OFF);

    (Feel free to make obvious assumptions, such as leftFront being an const uint_t, and modify to your favorite naming conventions and style guidelines. None of that is the point here.)

    Which code is better?   Well, from our intro to programming course probably we want to write the first type of code. It is more flexible, "elegant," and uses the oh-so-cool concept of iteration.  On the other hand, the second type of code is likely to get our fingers smacked with a ruler in a freshman programming class.  Where's the iteration?  Where is the flexibility? What if there are more than four items that need to be turned off? Where is the dazzling display of (not-so-advanced) computer science concepts?

    But hold on a moment. It's a four wheeled robot we're building.  Are you really going to change to a six wheeled robot?  (Well, maybe, and I've even worked a bit with such robots.  But sticking on two extra wheels isn't all that common after the frame has been welded!)  So what are you really buying by optimizing for a change that is unlikely to happen?

    Let's look at it a different way. Many times I'm not interested in elegance, clever use of computer science concepts, or even number of lines of code. What I am interested in is that the code is actually correct. Have you ever written the loop type of code and gotten it wrong?  Be honest!  There is a reason that "off by one error" has its own wikipedia entry. How long did it take you to look at the loop and make sure it was right? You had to assume or go look up the value of maxWheels, right?  If you are writing unit tests, you need to somehow figure out a way to test that all the motors got turned off -- and account for the fact that maxWheels might not even be set to the correct value.

    But, with the second set of code, it's pretty easy to see that all four wheels are getting turned off.  There's no loop to get wrong.  (You do have to make sure that the four wheel consts are set properly.)  There is no loop to test.  Cyclomatic complexity is lower because there is no looping branch.  You don't have to mentally execute the loop to make sure there is no off-by-one error. You don't have to ask whether the loop can (or should) execute zero times. Instead, with the second set of codes you can just look at it and count up that the wheels are being turned off.

    Now of course this is a pretty trivial example, and no doubt one that at least someone will take exception to.  But the point I'm making is the following.  The first code segment is what we were all taught to do, and arguably easier to write. But the second code segment is arguably easier to check for correctness.  In peer reviews someone might miss a bug in the first code. I'd say that missing a bug is less likely in the second code segment.

    Am I saying that you should avoid loops if you have 10,000 things to initialize?  No, of course not. Am I saying you should cut and paste huge chunks of code to avoid a loop?  Nope. And maybe you actually do want to use the loop approach in your situation even with 3 or 4 things to initialize for a good reason. And if you get lots of lines of code that is likely to be more error-prone than a well-considered loop. All that I'm saying is that when you write code, consider carefully the tradeoff between writing concise, but clever, code and writing code that is hard to get wrong. Importantly, this includes the risk of a varying style causing an increased risk of getting things wrong. How that comes out will depend on your situation.  What I am saying is think about what is likely to change, what is unlikely to change, and what the path is to least risk of having bugs.

    By the way, did you see the bug?  If you did, good for you.  If not, well, then I've already proved my point, haven't I?  The wheel number should start at 0 or the limit should be maxWheels+1.  Or the "<" should be "<=" -- depending on whether wheel numbers are base 0 or base 1.  As it is, assuming maxWheels is 4 as you'd expect, only 3 out of 4 wheels actually get turned off.  By the way, this wasn't an intentional trick on the reader. As I was writing this article, after a pretty long day, that's how I wrote the code. (It doesn't help that I've spent a lot of time writing code in languages with base-1 arrays instead of base-0 arrays.) I didn't even realize I'd made that mistake until I went back to check it when writing this paragraph.  Yes, really.  And don't tell me you've never made that mistake!  We all have. Or you've never tried to write code at the end of a long day.  And thus are bugs born.  Most are caught, as this one would have been, but inevitably some aren't.  Isn't it better to write code without bugs to begin with instead of find (most) bugs after you write the code?

    Instead of trying to write code and spending all our effort to prove that whatever code we have written is correct, I would argue we should spend some of our effort on writing code that can't be wrong.  (More precisely, code that is difficult to get wrong in the first place, and easy to detect is wrong.)  There are many tools at our disposal to do this beyond the simple example shown.  Writing code in a style that makes very rigorous static analysis tools happy is one way. Many other style practices are designed to help with this as well, chief among them being various type checking strategies, whether automated or implemented via naming conventions. Avoiding high cyclomatic complexity is another way that comes to mind. So is creating a statechart before actually writing the code, then using a switch statement that traces directly to the statechart. Avoiding premature optimization is also important in avoiding bugs.  The list goes on.

    So the next time you are looking at a piece of code, don't ask yourself "do I think this is right?"  Instead, ask yourself "how easy is it to be sure that it is not wrong?"  If you have to think hard about it -- then maybe it really is incorrect code even if you can't see a bug.  Ask whether it could be rewritten in a way that is more obviously not wrong.  Then do it.

    Certainly I'm not the first to have this idea. But I see small examples of this kind of thing so often that it's worth telling this story once in a while to make sure others have paused to consider it.  Which brings us to a quote that I've come to appreciate more and more over time.  Print it out and stick it on the water cooler at work:

    "There are two ways of constructing a software design: One way is to make it so simple that there are obviously no deficiencies and the other way is to make it so complicated that there are no obvious deficiencies."

            — C.A.R. Hoare, The 1980 ACM Turing Award Lecture  (CACM 24(2), Feb 1981, p. 81)

    (I will admit that this quote is a bit clever and therefore not a sterling example of making a statement easy to check for correctness.  But then again he is the one who got the Turing Award, so we'll allow some slack for clever wording in his acceptance essay.)

    Peer Reviews and Critical Software

    Every line of critical embedded software should be peer reviewed via a process that includes a physical face-to-face meeting and that produces an auditable peer review report.

    Consequences:
    Failing to perform peer reviews can reasonably be expected to increase the defect rate in software for several reasons. All real-world projects have limited time and resources, so by skipping or skimping on peer reviews developers have missed an easy chance to eliminate defects. With inadequate reviews, developers are spread thin chasing down bugs found during testing. Additionally, peer reviews can find defects that are impractical to find in most types of testing, especially in cases of fault management or handling unexpected/infrequent operating scenarios.

    Accepted Practices:
    • Every line of code must be reviewed by at least one independent, technically skilled person. That review must include actually reading the entirety of the code rather than just looking at selected portions.
    • Peer reviews must be documented so that it is possible to audit the fact that they took place and the effectiveness of the reviews. At a minimum this includes recording the name of the reviewer(s), the code reviewed, the date of the review, and the number of defects found. If no auditable documentation of software quality is available for incorporated components (e.g., safety certification or peer review reports), then new peer reviews must be performed on that third-party code.
    • Acceptable safety critical system software processes normally require a formal meeting-based review rather than a remote review, e-mail review, or other casual checking mechanism.

    Discussion:
    Peer reviews involve having an independent person – other than the author –  look at source code and other design documents. The main purposes of the review are to ensure that code conforms to style guidelines and to find defects missed by the author of the code. Running a static analysis tool is not a substitute for a peer review, and neither is an in-person discussion that solely discusses the output of a static analysis tool. A proper peer review requires having an independent person (or, strongly preferable, a small group of independent reviewers) read the code in its entirety to ensure quality. The everyday analogy to a peer review is having someone else proof-read something you’ve written. It is nearly impossible to see all our own mistakes whether we are writing software or writing English prose.

    It is well known that more formal reviews provide more efficient and effective results, with the gold standard being what is known as a Fagan Style Inspection (a “code inspection”) that involves a pre-review, a formal meeting with defined roles, a written review report, and follow up actions. Regardless of the type of review, accepted practice is to record the results of reviews and audit them to make sure every single line of code has been reviewed when written, and re-reviewed when a module has been modified.



    General code inspection process.

    MISRA requires a “structure program review” for SIL 2 and above. (MISRA Report 2 p. ix). MISRA specifically lists “Fagan Inspection” as a type of review (MISRA Software Guidelines p. 12), and devotes two appendices of a report on verification and validation to “walkthroughs,” listing structured walkthroughs, code inspections, Fagan inspections, and peer reviews (MISRA Report 6 pp. 132-136). MISRA points out that walkthroughs (their general term for peer reviews) “are acknowledged to be an effective process for identifying errors in programs – indeed they can be more effective than computer-based testing for certain types of error.”

    MISRA also points out that fixing a bug may make things worse instead of better, and says that code reviews and analysis should be used to validate bug fixes. (MISRA Report 5 p. 135)
    494. Peer reviews are somewhat labor intensive, and might account for 10% of the effort on a project. However, it is common for good peer reviews to find 50% or more of the defects in a code base, and thus finding defects via peer review is much cheaper than finding them via testing. Ineffective reviews can be diagnosed by the fact that they find far fewer defects. Acceptable peer reviews normally find defects that would be missed by testing, especially in parts of the code that are difficult to test thoroughly (for example, exception and failure management code).

    Selected Sources:
    McConnell devotes Chapter 24 to a discussion of reviews and inspections (McConnell 1993). Boehm & Basili summarized best practices for reducing software defects, and included the following point relevant to peer reviews: “Peer reviews catch 60 percent of the defects.”  (Boehm 2001, pg. 137).

    Ganssle lists four steps that should be the first steps taken to improve software quality. They are: “1. Buy and use a version control system; 2. Institute a Firmware Standards Manual; 3. Start a program of Code Inspection; 4. Create a quiet environment conducive to thinking.” #3 is his term for peer reviews, indicating his recommendation for formal code inspections. He also says that he knows companies that have made all these changes to their software process in a single day. (Ganssle 2000, p. 13). (Ganssle’s #2 item is coding style, discussed in Section 8.6. ).

    MISRA Software Guidelines list the following as techniques on a one-picture overview of the software lifecycle: “Walkthrough, Fagan Inspection, Code Inspection, Peer Review, Argument, etc.” (MISRA Guidelines 1994, pg. 20) indicating the importance of formal peer reviews in a safety critical software lifecycle. Integrity Level 2 (which is only somewhat safety critical) and higher integrity levels require a “structured program review” (pg. 29). That document also gives these rules: “3.5.2.2 Before dynamic testing begins the code should be reviewed in accordance with the software verification plan to ensure that it does conform to the design specification” (pg. 56) and “3.5.2.3 Code reviews and/or walkthroughs should be used to identify any inconsistencies with the specifications” (pg. 56) and “4.3.4.3 The communication of information regarding errors to design and development personnel should be as clear as possible. For example, errors found during reviews should be fully recorded at the point of detection.”

    MISRA C rule 116 states: “All libraries used in production code shall be written to comply with the provisions of this document, and shall have been subject to appropriate validation” (MISRA C pg. 55). Within the context of embedded systems, an operating system such as OSEK would be expected to count as a “library” in that it is code included in the system that is relied upon for safety, and thus should have been subject to appropriate validation, which would be expected to include peer reviews. If there is no evidence of peer review or safety certification, the system designer should perform peer reviews on the OS code (which is an excellent reason to use a safety certified OS!)

    Fagan-style inspections are a formal version of a “peer review,” which involves multiple software developers looking at software and other design artifacts to find defects. Fagan-style inspections originated at IBM (Fagan 1976). A later paper presented updated techniques, concluding that “inspections increase productivity and improve final program quality. Furthermore, improvements in process control and project management are enabled by inspections.” (Fagan 1986). It is widely recognized that Fagan-style inspections are a best practice, and that some sort of effective peer review technique is an accepted practice.

    Fagan-style Formal Inspections are recommended by the FAA (FAA 2000, p. J-23). IEC 61503-3 highly recommends performing some sort of design review on all software at all SILs, and recommends Fagan inspections at SIL4. (p. 91).

    References:
    • Boehm, B. & Basili, V., Software Defect Reduction Top 10 List, IEEE Computer, pp. 135-137, Jan. 2001.
    • FAA, System Safety Handbook, Appendix J: Software Safety, Federal Aviation Administration, Dec. 2000
    • Fagan, M., "Advances in software inspections," IEEE Trans. Software Engineering, SE-12, July 1986, pp. 744-751.
    • Fagan, M., "Design and code inspections to reduce errors in program development," IBM Systems Journal, 15(3), 1976, pp. 182-211.
    • Ganssle, J., The Art of Designing Embedded Systems, Newnes, 2000.
    • McConnell, Code Complete, Microsoft Press, 1993.
    • MISRA, (MISRA C), Guideline for the use of the C Language in Vehicle Based Software, April 1998.
    • MISRA, Development Guidelines for Vehicle Based Software, November 1994 (PDF version 1.1, January 2001).
    • MISRA, Report 2: Integrity, February 1995.
    • MISRA, Report 5: Software Metrics, February 1995.
    • MISRA, Report 6: Verification and Validation, February 1995.

    Peer Review Spreadsheet

    Summary: I've found peer reviews are only effective if they have tangible paperwork. Here's a minimalist approach that has worked for me in a classroom situation.



    Good peer reviews can be very effective at finding bugs .. but bad peer reviews can be nearly useless. I teach a course that involves a semester-long software project and found it really difficult to get students to do proper peer reviews.  Basically, they were going through the motions but didn't produce much (i.e., didn't find many bugs). The emphasis on the materials I provided them was checklists for what to look for, but not a lot of formality in reporting results, because I wanted to minimize paperwork.  That didn't work. Teams were only finding about 10% of their bugs in reviews, which is just not enough to be worth the effort.

    So one year I changed things, and made them to use a variant of the below spreadsheet to report results. The results were dramatic. The first semester I used the spreadsheet, defects found via peer review went from about 10% to about 50% across the class, and have stayed there ever since. (Also, the effort profile changed dramatically for the better, but that's a topic for another posting.) In my experience, finding 50% of bugs or so in peer review is about right. So, while I'm not claiming ultimate academic rigor for this study, it seems based on this experience that adopting a spreadsheet like this can be effective at improving the quality of peer reviews.

    Here is the spreadsheet (click to download the .XLS file):




    The first few lines are to record the project name, date, artifact (e.g., file name of the code being reviewed), and names of the reviewers present.  # Bugs is filled out later. The artifact author is intentionally omitted because the records are about finding bugs, not blaming the author. The issue rows are a place to record issues found in the review in free text, usually only one or two sentences apiece.

    The status fields are left blank during the review. However, within 24 hours after the review has taken place the author of the item being reviewed needs to update it to indicate "Fixed", "Not Fixed," or "Not a Bug." The idea here is that if it is easy to fix, the burden to record the bug and fix it is minimal -- it stays in the spreadsheet. But if a bug is too hard to fix immediately, it is "Not Fixed" and must be entered as a formal bug report into the bug tracking system (Bugzilla or otherwise). Some items turn out not to be bugs, and it is OK to record them as such (e.g., a feature request or a misunderstanding by the reviewers). When the program author has updated the status, the # Bugs line is updated to reflect the number of bugs actually found, and that number is rolled up to a project tracking spreadsheet.

    This last piece about rolling up the # Bugs to a higher level of visibility is crucial. In my course I have the Teaching Assistants track the number of bugs found weekly for every project team and make sure they asked hard questions if the answers were consistently zero. Really, that's all it took. Apparently if the teams know someone is watching, they'll look a bit harder to find bugs, and once they do it seems that the process boot-strapped into fairly effective reviews with minimal help from the course staff. It's worked several years in a row for me -- almost like a light switch had been flipped for the students in my class. Results have been pretty consistent since we started using the spreadsheet at about 50% of bugs found in peer reviews across dozens of teams. It should be noted that we peer review not only code, but also tests, statecharts, sequence diagrams, and other artifacts for our projects, and the payoff in finding bugs early has been unmistakable. Of course I need to make a "Your Mileage May Vary" disclaimer here, but it's worked for me.

    I'd be interested in hearing stories about simple ways to make reviews more effective from industry teams as well. Ideally each team gets solid training on a review process along with help on soft skills for review leaders. But realistically a lot of times a bunch of engineers are just tossed into a room and told to make it happen. Knowing tricks that help is not a bad thing.


    Top 10 Best Practices for Peer Reviews

    Here is a nice list of best practices for peer reviews from SmartBear. These parallel the recommendations I usually give, but it is nice to have the longer version readily available too (see link below).

    1. Review fewer than 200-400 lines of code at a time.
    2. Aim for an inspection rate of less than 300-500 LOC/hr (But, see comment below)
    3. Take enough time for a proper, slow review, but not more than 60-90 minutes
    4. Authors should annotate source code before the review begins
    5. Establish quantifiable goals for code review and capture metrics so you can improve your process
    6. Checklists substantially improve results for both authors and reviewers
    7. Verify that defects are actually fixed
    8. Managers must foster a good code review culture in which finding defects is viewed positively
    9. Beware the "Big Brother" effect  (don't use metrics to punish people)
    10. The Ego Effect: do at least some code review, even if you don't have time to review it all

    And now my comments:  The data I've seen shows 300-500 LOC/hr is too high by a factor of 2 or so. I recommend 100-200 lines of code per hour for 60-120 minutes. It may be that SmartBear's tool lets you go faster, but I believe that comes at a cost that exceeds the time save.

    I deleted their best practice #11, which says that lightweight reviews are great stuff, because I don't entirely buy it.  Everything I've seen shows that lightweight reviews (which they advocate) are better than no reviews, and for that reason perhaps they make a good first step. But if you skip the in-person review meeting you're losing out on a lot of potential benefit.  Your mileage may vary.

    You can get the full white paper here: http://support.smartbear.com/resources/cc/11_Best_Practices_for_Peer_Code_Review.pdf
     (They are not compensating me for posting this, and I presume they don't mind the free publicity.)

    Embedded System Code Review Checklist

    This past summer I had the opportunity to work with my friend Gautam Khattak on several industry design reviews. By the time the dust had settled, we'd had a lot of time to think about patterns of common problems and the types of things that designers should be looking for in peer reviews for embedded system software.

    As a result, we've written a two-page checklist for embedded system code reviews that you are free to use however you like.
    The sections include: function, style, architecture, exception handling, timing, validation/test, and hardware. This is a pretty thorough starting point, but no doubt you will want to tailor these to your specific situation.

    The checklists are broken into sections to make it easier to do perspective-based reviews. The idea is to divide up the sections among 3 or 4 reviewers so that each reviewer is thinking about somewhat different aspects of the code, resulting in better review coverage of potential issues.

    Everyone has their own favorite thing to look for in a review. If you think we've missed one please let us know with a comment. One thing we have intentionally left out is system-level problems that are better done in a system-level review. These are primarily intended for module reviews that look at a couple hundred lines of code at a time.

    Again, you can use these within your company or for any other purpose without having to ask our permission. But we'd be happy to hear from you if you've found them especially useful or if you have suggestions for things we've missed.

    Peer Reviews are Cheap

    In previous posts I've talked about how useful peer reviews can be. But some many folks say they are just too expensive. That just isn't so.  Here's the math.

    You can write good embedded software at the rate of 1-2 lines of code per hour. (That is all source lines of code divided by all hours for a project, including requirements, test, etc.   Really, that is the number.)

    Formal, rigorous, thorough peer reviews target 100-200 lines of code reviewed per hour (we're talking Fagan style inspections, which are quite formal, but still the most cost effective review method from all the data I've seen). Let's say you have 4 people in an inspection. That is 25-50 lines of code per person-hour.  If you include an hour of prep time for each person for a two-hour review, that is still 17-33 lines of code per person-hour.

    Think you write code faster?  OK, let's say you're Agile and do 3 lines of code/hr (what I've seen in industry, and comes with increased risk we can discuss elsewhere).  That's still nowhere near the review productivity rate.

    We're talking maybe 5%-10% of project effort to do heavy-weight reviews.  And it generally finds half the bugs. More importantly, it finds them early, so you don't have a lot of rework.

    So why does everyone say it is too expensive?

    Peer Reviews and the 50/50 Rule

    Surprisingly, there is a fairly easy way to know if your peer reviews are effective. They should be finding about 50% of your defects, and testing should be finding the other 50%. In other words the 50/50 rule for peer reviews is they should find half the defects you find before shipping the product.  If peer reviews aren't finding that many defects, something is wrong.

    I base this rule of thumb on some study data and my observations of a number of real projects. If you want to find out if teams are really doing peer reviews (and doing ones that are effective), just ask what fraction of defects are found in peer reviews. If you get an answer in the 40%-60% range probably they're doing well.  If you get a lower answer than 40%, peer reviews are being skipped or are being done ineffectively. If they answer is "we do them but don't log them" then most of the time they are being done ineffectively, but you need to dig deeper to find out what is going on.

    If you are trying to find all your defects in test (instead of letting peer review get half of them for you), you are taking some big risks. Test is usually a more expensive way to find defects. More importantly, peer review tends to find many defects or poor design choices that are difficult to find by testing with any reasonable effort.

    So, why make your testing expensive and your product more bug prone? Try some peer reviews and see what they find.

    Embedded Software Risk Areas -- Verification & Validation

    Series Intro: this is one of a series of posts summarizing the different red flag areas I've encountered in more than a decade of doing design reviews of industry embedded system software projects. You can read more about the study here. If one of these bullets applies to your project, you should consider whether that presents undue risk to project success (whether it does or not depends upon your specific project and goals). The results of this study inspired the chapters in my book.

    Here are the Verification and Validation red flags:
    • No peer reviews
    Code, requirements, design and other documents are not subject to a methodical peer review, or undergo ineffective peer reviews. As a result, most bugs are found late in the development cycle when it is more expensive to fix them.
    • No test plan
    Testing is ad hoc, and not according to a defined plan. Typically there is no defined criterion for how much testing is enough. This can result in poor test coverage or an inconsistent depth of testing.
    • No defect tracking
    Defects and other issues are not being put into a bug tracking system. This can result in losing track of outstanding bugs and poor prioritization of bug-fixing activities.
    • No stress testing
    There is no specific stress testing to ensure that real time scheduling and other aspects of the design can handle worst case expected operating conditions. As a result, products may fail when used for demanding applications.