F05SENG411 Code Reviews
From Craig
These notes represent a summary of Chapter 24 from Code Complete, Steve McConnell, Microsoft Press, 1993
| Table of contents |
|
2.1 Reviews in relation to other aspects of SQA |
What is the purpose of a Code Review?
- First and foremost, the reason for doing a code review is to improve the quality of the code being reviewed
- Improve programmers
What a code review is not
- A meeting to negatively criticize someone else's code
- A meeting to criticize architecture or design (unless it is an architectural or design review)
- A meeting to criticize a colleague
- A meeting to determine whether someone is to be removed from a project or not
- A meeting to determine whether someone is performing up to standards
Role of Reviews in Software Quality Assurance
- All reviews are based on the notion that people can be un-critical about some aspects of their own work
- Other people don't necessarily have this limitation
- Sometimes, simply explaining the problem to someone else is enough to illuminate the problem itself.
Reviews in relation to other aspects of SQA
- Average rate of defect detection using Unit tests is 25%
- Average rate of defect detection as part of Integration Testing is 45%
- Average rate of defect detection as part of code and design inspections is 55-60%
- Code reviews act as a complement to other techniques
Case Studies on code reviews
- 11 programs developed by the same group of people
- 5 were developed without review
- 6 were developed with review
- Error rates
- 4.5 errors/100 lines of code (without review)
- 0.82 errors/100 lines of code (with review) 80% reduction.
- IBM's 500,000 line Orbit project
- Used 11 levels of inspection
- Delivered with an error rate of 1% of expected.
- AT&T, 200 person organization
- Recorded 14% increase in productivity
- 90% decrease in defects
- JPL estimates it saves $25,000 per inspection by finding and fixing defects at an early stage
Dissemination of Information
- For programmers to improve as programmers, they need to be exposed to good code.
- They also need to be exposed to the processes which go into creating good code.
- Coding standards are useful, but if they are not followed how effective can they be?
- Need a forum to encourage discussion and encourage the use of these standards.
- Need a mechanism for feedback.
- Is the technical work being done?
- Is the technical work being done well?
Code Inspections
- An inspection is a specific kind of review
- Developed by Michael Fagan and used at IBM for several years before being published
- Key aspects of Code Inspections
- Checklists focus the reviewers' attention on areas that have been problems in the past
- The emphasis is on defect detection, not correction
- Reviewers prepare for the inspection beforehand and produce a list of problems they've discovered
- Distinct roles are assigned to the participants
- The moderator of the inspection isn't the author of the work product being inspected
- The moderator may have special training in moderating inspections
- Data collected at each inspection is used in future inspections (to improve process)
- General management doesn't attend inspection meetings. Technical management are likely to attend.
Roles during an inspection
- Moderator
- Keeps the inspection on track
- Manages the inspection (schedule, booking meeting room, distributing code, etc)
- Author
- The person who wrote the code under review
- Plays a minor role in the inspection
- Author may make a presentation of the code before the inspection takes place
- Reviewer
- Reviews code, but cannot be the author
- Goal: find defects
- Scribe
- Records important information from the meeting (such as defects)
- Reviewers and authors should not take on this role
Management should not be present at a code inspection. It is purely a technical review.
Procedure for an inspection
- Planning
- Author gives code to moderator
- Moderator decides who will inspect the code and when inspection will occur
- Distributes code and checklists
- Overview
- If reviewers aren't familiar with material, the author will make a presentation about the project.
- The code should speak for itself, so any discussion make by the author should be limited.
- Preparation
- Each reviewer takes 90 minutes to become familiar with the design or code.
- Reviewer uses checklists to focus attention of review
- The amount of code which can be reviewed in 90 depends on many factors
- Inspection meeting
- Author presents the code/design
- Logic is explained
- Scribe records errors. Discussion of an error stops as soon as it is recognized as an error
- Scribe records type and severity
- Don't discuss solutions during meeting
- Meeting should not last more than 2 hours
- Inspection report
- Within a day, moderator produces a report that lists each defect
- Keep records on the number of defects found. This way, the effectiveness of inspections can be verified at a later date
- Rework
- Moderator assigns the defects to someone, usually the author
- Follow-up
- Moderator is responsible for seeing that all of the rework assigned during inspection is complete
- If more than 5% of the design or code needs to be reworked, the whole inspection process should be repeated.
- Third-hour meeting (optional)
- Solutions shouldn't be discussed as part of the inspection.
- You can hold an optional meeting after the inspection to discuss solutions
Fine Tuning the inspection
Once you become more skilled at running code inspections, you can usually find ways to improve them. Be careful when you modify the inspection process that you don't remove important components. You should be able to determine whether changes you've made to the process have improved or degraded the process.
Teamwork issues
- The experience of a code review/inspection should be a positive one for the author
- Not a process attemption to determine who is right and who is wrong
- Should not criticize the author
- Any inappropriate comments should be made clear by the moderator
- From the author's perspective:
- The author will hear criticisms about defects (which aren't really defects) and defects which are debatable
- This is not the forum for debate.
- The author should acknowledge the criticism and move on. The author should not defend the design/code
- The author can think about each point in private and decide whether it's valid or not.
- From the reviewer's perspective
- Reviewers must remember that it is the author who has the ultimate responsibility for deciding what to do about the defect.
- Input may appear from senior technical people.
Inspection Checklist
- Do you have checklists that focus reviewer attention on areas which have been problems in the past?
- Is the emphasis on detection rather than correction?
- Are inspectors given enough time to prepare for the inspection? Is each one prepared?
- Does each participant have a role to play? Is each participant aware of the role he/she is playing?
- Does the meeting move at a productive rate?
- Is the meeting limited to 2 hours?
- Is the moderator trained in facilitating inspections?
- Is data about defects being collected so that it can be used to improve future inspections?
- Are the results of the inspection followed-up?
- Does management understand the purpose of these meetings?
Less formal reviews
Walkthroughs
- Popular because of low overhead
- Varies from formal to informal.
- Can be moderated by the author of the code
- Purpose is to improve technical quality of the code rather than to assess it.
- Senior programmers can use this as a chance to pass on experience to less senior people
Comparison: Inspections vs. Walkthroughs
| Property | Inspection | Walkthrough |
|---|---|---|
| Formal moderator training | Yes | No |
| Distinct participant roles | Yes | No |
| Who runs the inspection or walkthrough | Moderator | Author |
| Checklists | Yes | No |
| Focused review effort? ie. looking for specific kinds of errors | Yes | No |
| Formal follow-up | Yes | No |
| Fewer future errors because of detailed error feedback to individual programmers | Yes | Incidental |
| Improved inspection process from analysis of results | Yes | No |
Code Reading
- Alternative to inspections and walkthroughs
- Read code and look for errors
- Can also comment on quality aspects such as design, style, readability, maintainability, etc.
- Studies have shown that code reading can find 20-60% more errors than testing can
Code reading process
- Author hands out code (typically between 1000 and 10000 lines)
- 2 or more people read code
- Code is read by reviewers (usually 1000 lines/day)
- When reading is finished, meeting is hosted by author. Usually lasts between 1-2 hours
Useful Links
Mozilla Code Review FAQ (http://www.mozilla.org/hacking/code-review-faq.html)
Macadamian Code Review Checklist (http://www.macadamian.com/index.php?option=com_content&task=view&id=27&Itemid=31)
Practical Suggestions for Code Reviews (http://www.oualline.com/col/review.html)
Wikipedia Entry on Software Inspection (http://en.wikipedia.org/wiki/Software_inspection)
