APB-Code review
Code review
Keywords: Code review sessions - conduct and expectations
Contents
This unit is under development. There is some contents here but it is incomplete and/or may change significantly: links may lead to nowhere, the contents is likely going to be rearranged, and objectives, deliverables etc. may be incomplete or missing. Do not work with this material until it is updated to "live" status.
Abstract
This page presents conduct and expectations for code reviews.
This unit ...
Prerequisites
This unit has no prerequisites.
Objectives
...
Outcomes
...
Deliverables
- No deliverables: This unit has no deliverables.
Contents
In this course we will try "Code Reviews for Credit". This means, we are not going to evaluate the "code" - i.e. the learning units, but the reviewers. A well-prepared, thoughtful, knowledgeable review will provide immensely valuable feedback, I hope that reviewing for credit will help the reviewers put their heart into it.
Schedule
- Units are to be kept in the state that was submitted when they were due, until after the code review.
- We will use four weeks for reviews. We will schedule six reviews per class session, with three to four reviewers. This will give everyone approximately five minutes for questions, proposals and feedback.
- Everyone will be involved at every class session.
Why the review?
Code and design review
Preparation
- The entire class is expected to have carefully worked through the learning units that are scheduled for review, so everyone can follow the discussion.
- The reviewers have worked through the unit in detail and lead the discussion.
- Code will not be presented by the author like in a normal code review, but the reviewers may ask some initial questions for clarification.
- How does the unit fulfil its own requirements and objectives?
- How does the unit contribute to the objectives of the course?
- Is the unit well integrated with others?
- What design decisions were made? How did they work out?
- Do the examples illustrate the concepts?
- Are there unstated assumptions, hidden dependencies, obstacles to refactoring?
- Is it correct?
- Does the form and coding style need improvement?
- Is the sample code robust against faulty input?
- Does the example code
- is the code safe against overwriting data?
- Are the tasks meaningful? Have sample solutions been provided? Are they commented to the degree that is appropriate for someone who needs o look up the sample solution?
- During the review, reviewers take notes of responses to the issues they raised and their suggestions.
- After the review, on the same day, the reviewers summarize their issues and proposals on the "Talk" page of the unit (point form); once all suggestions are in, the unit author begins the revision. Note: it is not mandatory that the revisions follow the reviewers' suggestions - in the end, the author is responsible for their unit.
- Briefly consider improvements to coding style as suggestions but don't spend too much time on them (don't create a "Bicycle Shed" anti-pattern) - style is not the most important thing about the review. Be constructive and nice - you should encourage your colleagues, not demotivate them.
- Spend most of the time discussing architecture: how does this code fit into the general lay of the land? How would it need to change if its context changes? Is it sufficiently modular to survive? What does it depend on? What depends on it? Does it apply a particular design pattern? Should it? Or has it devolved into an anti-pattern?
- Focus on tests. What is the most dangerous error for the system integrity that the code under review could produce. Are there tests that validate how the code deals with this? Are there tests for the edge cases and corner cases[1]? This is the best part about the review: bring everyone in the room on board of the real objectives of the project, by considering how one component contributes to it.
- Finally, what's your gut feeling about the code: is there Code Smell? Are there suboptimal design decisions that perhaps don't seem very critical at the moment but that could later turn into inappropriate technical debt? Perhaps some refactoring is indicated; solving the same problem again often leads to vastly improved strategies.
Overall be mindful that code review is a sensitive social issue, and that the primary objective is not to point out errors, but to improve the entire team.
Further reading, links and resources
Notes
- ↑ A software engineer walks into a bar and orders a beer. Then he orders 0 beers. Then orders 2147483648 beers. Then he orders a duck. Then orders -1 beers, poured into a bathtub. Then he returns a beer he didn't order. Then he spills his beer on the floor, shrieks wildly and runs away without paying.
If in doubt, ask! If anything about this learning unit is not clear to you, do not proceed blindly but ask for clarification. Post your question on the course mailing list: others are likely to have similar problems. Or send an email to your instructor.
About ...
Author:
- Boris Steipe <boris.steipe@utoronto.ca>
Created:
- 2017-10-13
Modified:
- 2017-10-13
Version:
- 1.0
Version history:
- 1.0 First final version
This copyrighted material is licensed under a Creative Commons Attribution 4.0 International License. Follow the link to learn more.
