SOFT2201/COMP9201: Software Design and Construction 1
Code Reviews
Dr Xi Wu
School of Computer Science
The University of Sydney
Page 1
Announcement
– We will do an interactive code review guided by your tutor during this week’s tutorial
– Pair programming will be used during the tutorial and you can have a quick look at the week-8-preview- recommendation page on canvas (under module 8) before you go to your tutorial
– All programming as well as code review will be done on Ed workspace instead of your personal computer. Workspace will be open a few minutes before tutorial starts by your tutor
The University of Sydney Page 2
Outline
– Why review
– What to review
– What is a review
– What is a formal review
– What is an informal review The University of Sydney
Page 3
Why review?
The University of Sydney Page 4
Why review?
– What is the software, or aspect of the software, to be reviewed for?
– What are the specifications that need to be met?
– Does the software, or aspect thereof, being reviewed,
meet those specifications?
The University of Sydney
Page 5
What is the software for and does it work?
– What is the software, or aspect of the software, to be reviewed for?
– What are the specifications that need to be met?
– Does the software, or aspect thereof, being reviewed,
meet those specifications?
– We need a way to check that the software is appropriately designed to meet the expected criteria
The University of Sydney
Page 6
What to review?
The University of Sydney Page 7
What to review?
– Any part of a software project can be reviewed
– Code
– Documentation – Processes
– Management
– Specifications – Planning
The University of Sydney
Page 8
What is a review?
The University of Sydney Page 9
What is a review?
– Asoftwarereviewis
“A process or meeting during which a software product, set of software products, or a software process is presented to project personnel, managers, users, customers, user representatives, auditors or other interested parties for examination, comment or approval.”
IEEE Standard 1028-2008, “IEEE Standard for Software Reviews”, clause 3.5
The University of Sydney Page 10
What is a formal review?
The University of Sydney Page 11
Fagan inspection
– An early effort to formalise the process of reviews
– The basis, or at least similar, to subsequent formal
approaches
– Describe program development process in terms of operations
– Define ‘entry’ and ‘exit’ criteria for all operation
– Specify objectives for the inspection process to keep
the team focused on one objective at a time
The University of Sydney
Page 12
Fagan inspection
– Classify errors by type, rank by frequency, identify which types to spend most time looking for
– Analyse results and use for constant process improvement
The University of Sydney
Page 13
Fagan inspection operation
– Specify objectives for the inspection process to keep the team focused on one objective at a time
– Planning
– Overview
– Preparation – Inspection – Rework
– Follow-up
The University of Sydney
Page 14
Fagan inspection operations
– Planning
– Preparation of materials
– Arrangingofparticipants
– Arrangingofmeetingplace
– Overview
– Group education of participants on review materials
– Assignmentofroles
The University of Sydney
Page 15
Fagan inspection operations
– Preparation
– Participants review item to be inspected and supporting material to prepare for the meeting, noting any questions or possible defects
– Participants prepare their roles – Inspection meeting
– Actualfindingofdefect
The University of Sydney
Page 16
Fagan inspection operations
– Rework
– Rework is the step in software inspection in which the defects found during the inspection meeting are resolved by the
author, designer or programmer. On the basis of the list of defects the low-level document is corrected until the requirements in the high-level document are met.
– Follow-up
– In the follow-up phase of software inspections all defects found in the inspection meeting should be corrected. The moderator is responsible for verifying that this is indeed the case. They should verify that all defects are fixed and no new defects are inserted while trying to fix the initial defects.
The University of Sydney Page 17
Formal inspection
– Management review
– Monitor progress
– Determine status of plans
– Evaluate management effectiveness
– Supports decisions about changes in direction, resource allocation, and scope
The University of Sydney
Page 18
Formal inspection
– Management review
– Maintenance plans
– Disaster recovery
– Migration strategies
– Customer complaints
– Risk management plans –…
The University of Sydney
Page 19
Formal inspection
– Management review roles
– Decision maker
– Review leader
– Recorder
– Management staff – Technical staff
The University of Sydney
Page 20
Formal inspection
– Management review processes
– Preparation
– Plan time and resources
– Provide funding
– Provide training
– Ensure reviews are conducted – Actonreviews
The University of Sydney
Page 21
Formal inspection
– Technical review
– Software requirements
– Software design
– Software test documentation – Specifications
– … procedures
The University of Sydney
Page 22
Formal inspection
– Technical review roles
– Decision maker
– Review leader
– Recorder
– Technical reviewer
The University of Sydney
Page 23
Formal inspection
– Inspections
– The purpose of an inspection is to detect and identify software product anomalies. An inspection is a systematic peer examination that does one or more of the following:
• Verifies that the software product satisfies its specifications
• Verifies that the software product exhibits specified quality
The University of Sydney
Page 24
attributes
• Verifies that the software product conforms to applicable regulations, standards, guidelines, plans, specifications, and procedures
External Audits
– Reviews may also be performed by external groups
– Aformalprocessisgenerallyhighlydesirablewhendealing with external groups and may extend any of the above approaches
The University of Sydney
Page 25
What is an informal review?
The University of Sydney Page 26
Informal reviews
– Reviews may be far less formal – Pair-programming
– “Over the shoulder”
– Walkthroughs
– Presentations
– Self-guided reviews – Checklists
The University of Sydney
Page 27
Informal reviews
– Formal reviews are far more effective than informal reviews
– Informal reviews are far more convenient than formal reviews
The University of Sydney
Page 28
How about a semi-formal code review?
The University of Sydney Page 29
Purpose
– Have a clear purpose for the review
– Identify all relevant project specifications
– Identify how the specifications can be verified – Identify who the relevant stakeholders are
The University of Sydney
Page 30
Change List/Pull Request reviews
– Acomplete,‘working’projectexists
– Asetofchangesaretobeapplied
– The changes must be reviewed before they are accepted
The University of Sydney
Page 31
Change List/Pull Request reviews
– Owner
– Is this change wanted?
– Technical reviewer
– Does this change work?
– Technical reviewer
– Is this change maintainable?
The University of Sydney
Page 32
• (Can I understand it?)
A brief visit to Design by Contract (DbC)
– Asoftwaredesignapproachforprogramcorrectness
– Also known as contract programing, programming by
contract, design-by-contract programming
– Definition of formal, precise and verifiable interface specification for software components
– Pre-conditions, postconditions and invariants (contract) The University of Sydney
Page 33
Reviewing DbC change lists
– Definition of formal, precise and verifiable interface specification for software components
– Pre-conditions, postconditions and invariants (contract)
– Are the pre-conditions met?
– Are the post-conditions met?
– Are the invariants invariant?
– Are these likely to stay so?
– How hard is it to verify?
The University of Sydney
Page 34
Change List/Pull Request reviews
– Owner
– Does the documentation agree with the specifications?
– Technical reviewer – Aretheretests?
– Do the tests verify the pre/post conditions are met?
– Technical reviewer
– Is the code written according to the style guide? – Does the code use appropriate design patterns?
The University of Sydney
Page 35
Change List review challenges
– Size
– The more there is to read, the more scope there is to miss
problems
– Many small code reviews are more manageable (somewhat like unit tests vs blackbox system tests)
– Alargecodereviewcantakesomuchtimethatspecial planning is needed
– May need to place limits on acceptable sizes for review
The University of Sydney
Page 36
Change List review challenges
– Scope
– Changes that affect many processes may need more
reviewers for the required technical knowledge
– May sometimes be helped by multiple, smaller changelists
– May bring specifications from different processes into conflict, requiring management review
The University of Sydney
Page 37
• May also be triggered by small changes with big impacts, such as changing JDK version
Change List review challenges
– Complexity
– Fast, efficient, code may be hard to read and hard to
understand
– Is the code complexity, thus review complexity and maintenance complexity, worth the improvement?
– Does the complexity affect maintainability and testability?
The University of Sydney
Page 38
Change List review challenges
– Confusion
– What is the change meant to be for?
– May be doing too many unrelated things
– May be making unnecessary changes
– May even be about a disagreement between colleagues (my approach is better!)
The University of Sydney
Page 39
Change List review techniques
– Formalise the review process
– Allchangesmustbereviewed
– We already have review roles – Owner, Readability, Technical
– The planning and preparation are only needed once per project (plus for each major change in direction)
– The project needs clear specifications, requirements, and sufficient documentation
– TheCLmustmeettherequirements
– Changes are recommended and either acted on or disagreed
with and the result re-evaluated
The University of Sydney Page 40
Change List review comments
– Be clear
– Be objective
– State the issue
– State what is needed to fix it
– E.g., “Document this method”
– E.g., “These braces aren’t needed, please remove” – E.g., “Split this file into one per class, for readability” – E.g., “Use informative variable names”
The University of Sydney
Page 41
Change List review examples
– This needs tests
– This needs unit tests
– This needs integration tests
– This needs documentation
– What is this for?
– This doesn’t follow the style guide – Resubmit without the temp files
The University of Sydney
Page 42
Change List review techniques
– Automation is great! – When it works
– Code style
– Coverage
– Test suites
– Performance tests
– Spelling (A little more challenging in code) – Common code issues
The University of Sydney
Page 43
Reviewing larger projects
– You may need to review larger projects, rather than changes.
– Aformalreviewprocesswillensureallparties understand what is expected
– The code inspection will be similar to a changelist review, but with no existing base to compare to
– More work is needed to verify requirements
The University of Sydney
Page 44
Change List review comments
– E.g., “Document this method”
– E.g., “These braces aren’t needed, please remove”
– E.g., “Split this file into one per class, for readability”
– E.g., “Use informative variable names”
– Areportonasetofchangeswoulddiscusswhythe comments were made, the benefits, the problems, and discuss other approaches
– Acodereviewisfarmoreconciseanddirected
The University of Sydney Page 45
Task for Week 8
– Additional presentation video about code review on project can be found on canvas (Recorded Lecture section)
– Submit weekly exercise on canvas before 23.59pm Sunday
– Attend tutorial on time for an interactive code review guided
by your tutor during tutorial
– Attend Helpdesk session if you have any questions/difficulties on implementation perspective
The University of Sydney Page 46
What are we going to learn next week?
– Structural Design Pattern – Adapter
– Behavioral Design Pattern – Observer
The University of Sydney
Page 47
References
– Fagan, M. E. (1976). “Design and code inspections to reduce errors in program development”. IBM Systems Journal. 15 (3): 182–211. doi:10.1147/sj.153.0182
– IEEE Std . 1028-1997, “IEEE Standard for Software Reviews“, doi:10.1109/IEEESTD.2008.4601584
The University of Sydney Page 48