Reviews

Review What?

Review Everything

• Vision & Scope Document
• Requirements Specification
• Project Plan
• Design - High Level and Detailed Design
• Code
• Test Plan
• Documentation

Why Review?

1. Save time.

2. Save money.

3. Gain approval & increase sense of shared ownership.

4. Reviews find more defects than testing.

5. Share knowledge.

6. More ideas make better results.

Kinds of Reviews

​

1. Inspection
2. Code Review - Inspection of Code
3. Walk-through
4. Desk check
5. Self-review

Formal

Informal

Which Review to Use?

Source: Prof. Claude Laporte, U. of Quebec, Dept of Software and IT Engineering

Inspection

The most formal kind of review.

Purpose: find defects.

How To:

1. Choose work product to inspect.

2. Choose 4-5 people, including a moderator

3. Prepare: Everyone reads the work product in advance and notes suspected defects.

4. Inspection meeting: confirm defects & log them

Inspections may proposal correction (e.g. words in document)

5. Rework: author fixes defects from inspection log

Inspection Team

Author of document or work product

Project manager - for project documents

Representative of groups affected by the document, e.g. developers, management, PPQA person.

Inspectors should

• be familiar enough with project to understand problems and propose changes
• provide different perspectives on work product

​

Inspection Meeting

Moderator guides inspectors through work product.

Ask inspectors for defects.

Other inspectors (and author) confirm each defect, or explain why they disagree.

Inspectors agree on a fix (for document) or leave it to author to fix (code).

Record each defect in a written log.

​

​

After the Inspection

Rework: author fixes the work product.

​

Follow-up: inspectors individually review the revised work and approve or not approve it.

​

Acceptance: once all inspectors approve, the work product is accepted.

Inspection is NOT...

• Review of style
• Attempt to improve or optimize design*
• Evaluation of the author
• Subjective evaluation of quality

​

​

*Infosys: inspectors use a separate form to record comments, offer insights and ideas.

Is Inspection Worth the Time?

Inspection involving 5 people takes 10-20 man-hours, about half the time is preparation.

​

Inspection finds 5 - 10 defects, on average.

Source: Ron Radice, ‘Software Inspections: Past, Present, and Future.’, Software Technology Conference, Salt Lake City, Utah, May, 2001

�

Activity

Defect Injection and Detection

0

5

10

15

20

25

START

REQ

HLD

LLD

CODE

UT

IT

ST

SHIP

Defects/KLOC

Software Development Spending Profiles

Source: IEEE Software Engineering

Cost/Benefit of Inspections

Req. Design Code Test Post-Release

Rework Effort

Before Review/Inspection

After Implemented Review/Inspection

8%

1%

12%

3%

19%

4%

Formal Review/Inspection increased design effort by 4%

Decreased rework effort by 31%

Reduce 31%

in rework

Source: Vu, J., ‘Software Process Improvement Journey’, 8th Software Engineering Process Group Conference San Jose, California March, 1997.

Conclusion

Inspection Saves Time & Money

How Much Time Does it Take?

Source: Radice, ‘High Quality Low Cost Software Inspections’, 2002.

Code Review

An inspection of code.

Similar to Inspection but more time is spent on alternatives and qualitative issues.

Before Review:

- choose the code to review (you can't review everything this way) - see Stellman & Greene

- choose moderator, reader, and inspectors

- choose a date/time and duration (60-90 minutes)

- everyone reviews code individually and makes notes of issues they find (paper or online notes)

​

Code Review Meeting

During Review:

- the "reader" walks through the code aloud -- by section (class, method, code block), not literally reading code.

- Inspectors:

• ask about anything they don't understand
• question correctness of code
• suggest "better" or more self-explanatory alternatives

- Moderator: keep the review on track. Don't get bogged down discussing specific design or code issues.

- Recorder: writes down issues for follow-up

Code Review Follow-up

After Review:

- author addresses all issues, either revise code or explain to reviewer why he things no rework is needed

- do it promptly!

- gain agreement to close all issues

​

Product & Process Quality Assurance (PPQA):

- verify all issues were recorded

- verify all issues were fixed or addressed, not just closed

​

​

​

Code Review vs. Inspections

Code Review results in more open issues.

May refactor the code during the meeting... if it makes it easier to review.

May discuss alternatives or improvements.

Follow-up and consensus may be done online.

​

​

​

Walk-through

Author "walks" a small group through a work product.

More informal than code review and led by author.

Procedure is more flexible than inspections.

Can be done for code, design docs, use cases, test plan

​

​

​

Goals of Walk-through

​

• find defects
• get feedback and ideas
• discover other solutions
• help everyone understand the work product
• improve knowledge & skill
• shared ownership

​

​

​

Desk Check

Purpose

Individually review of code by another developer.

Usually done individually, with follow up discussion.

​

Procedure

A developer asks another developer to review his work.

The reviewer (at his own desk) checks the work and reports defects, questions, and suggestions for alternatives or improvement.

​

Git Pull Request

Purpose

Request review of work before incorporating it into a main "dev" branch or master branch.

​

A kind of "desk check" using Github or Bitbucket.

​

Tutorial: https://yangsu.github.io/pull-request-tutorial/

​

Guide: https://help.github.com/articles/using-pull-requests/

​

Example (JQuery):

https://github.com/jquery/jquery/pull/1051#discussion-diff-2287441

​

Self-Review

Always review your own work

​

Obvious, but often not done.

​

How to:

take a break before review. This is required.

decide what criteria you are going to use (what are you checking for?)

allocate sufficient time

record defects you find

Scripts and Checklists

Scripts and Checklists save time & make results more consistent.

How Save time?

- don't re-discover what you did before

- focus on the creative, not the routine

​

Script - describe the activity, its purpose, desired result, important steps, and "exit criteria".

​

Checklist - concise list of particular things to do or inspect

Script

Purpose: Find defects in code

Entry criteria: Code specification and design

Source code with tests that all pass.

Goal for Code Review: review why?

Checklist

Steps: 1.

2.

3.

Exit criteria: source code completely reviewed.

written record of all defects, suggestions, and open issues

​

PSP Code Review Script

Checklist

Reviews should use a checklist.

​

Contents of checklist depend on kind of thing being inspected!

​

Self-review and desk check are more effective if you use a checklist.

​

​

• RS 1 (TESTABLE) – All requirements are verifiable (objectively)
• RS 2 (TRACEABLE) – All requirements must be traceable to a systems specification, contractual/proposal clause.
• RS 3 (UNIQUE) – Requirements must be stated only once
• RS 4 (ELEMENTARY) – Requirements must be broken into their most elementary form
• RS 5 (HIGH LEVEL) – Requirement must be stated in terms of final need, not perceived means (solutions)
• RS 6 (QUALITY) – Quality attributes have been defined.
• RS 7 (HARDWARE) – Is hardware environment is completely defined (if applicable).
• RS 8 (SOLID) – Requirements are a solid base for design

Example Checklist for Requirements Specification (RS)

Source: Gilb, T., Graham, D., ‘Software Inspection’, Addison Wesley, 1993.

• CC1 (COMPLETE) - Verify that the code covers all the design. 
• CC2 (INCLUDES) - Verify that includes are complete.
• CC3 (INITIALIZATION) - Check variable and parameter initialization.
• CC4 (CALLS) - Check function call formats
• CC5 (NAMES) - Check name spelling and use
• CC6 (STRINGS) Check that all strings are ...
• CC7 (POINTERS) - Check that:
• Pointers are initialized to NULL,
• Pointers are deleted only after new, and
• New pointers always deleted after use.
• CC8 (OUTPUT FORMAT) - Check the output format:
• Line stepping is proper.
• Spacing is proper.
• CC9 (PAIRS) - Ensure the { } are proper and matched.
• CC10 (LOGIC OPERATORS) - Verify that the proper use of ==, =, //, and so on.

Example Checklist for C++ Code (CC)

Adapted from: Humphrey, W., ‘Introduction to the Personal Software Process’, Addison Wesley, 1997.

Another Code Inspection checklist

PSP Advice for Checklist

Watts Humphrey's advise:

​

1. Keep your checklist simple and short.

2. Must be complete (everything you actually ckeck for).

3. Group items into categories.

4. Tailor to the programming languages you use.

5. Address the kind of defects you inject. Not stuff that doesn't apply to your team.

6. Evolve. Revise it to be more effective.

​

​

Example Checklist for Java

Summary

1. Review Everything - not just code

2. Choose the appropriate level of review

3. Create a written result - not just talk

• result is online where everyone can see it
• open issues for specific items

4. Follow up & resolve all issues, answer all questions

5. Use tools to automate routine stuff (style checking ...)

6. Scripts and checklists make reviews more effective

Questions

1. Look at the PSP Code Review Checklist.

what categories do not apply to Python?

what categories can be done by automated tools?

​

​

​

References

Stellman & Greene, Applied Software Project Management, chapter 5 on Reviews.

- chapter 5 is available online.

​

Ship It! Item 13 - Review all Code

Practical advise for code reviews.

• review only a small amount of code
• one or two reviewers at most
• review very frequently, often several times per day

References

Karl Weigers, Peer Reviews in Software - A Practical Guide. Considered the "bible" on peer reviews.

Karl Weigers, Improving Quality Through Software Inspections article online.

https://www.processimpact.com/articles/inspects.pdf

Tom Gilb, Software Inspection.

R. Radice, High Quality Low Cost Software Inspections.

​

Acknowlegement

Some of these slides are from a workshop at NECTEC by Prof. Claude Leporte, U. of Quebec.

​