Code Reviewing
In this course, you will read your classmates’ code and give them comments about it. This document describes the whys and hows of the 6.102 code reviewing process.
You can’t learn how to write without learning how to read – and programming is no exception. Code reviewing is widely used in software development, both in industry and in open source projects. Many companies have instituted review-before-commit as a required policy. For example, you can’t get a line of code into the Google source code repository unless another Google engineer has read it, given feedback on it, and signed off on it.
There are several benefits of code review in practice. Reviewing helps find bugs in a way that’s complementary to other techniques (like static checking, testing, assertions, and reasoning). Reviewing uncovers code that is confusing, poorly documented, unsafe, or otherwise not ready for maintenance or future change. Reviewing also spreads knowledge through an organization, allowing developers to learn from each other through explicit feedback and by example. So code reviewing is not only a practical, important skill that you will need in the real world, but also a learning opportunity.
What to look for
Although you can make comments about anything you think is relevant, the primary goal of this class is to learn how to write code that is safe from bugs (SFB), easy to understand (ETU), and ready for change (RFC). Read the code with those principles in mind. Here are some concrete examples of problems to look for:
- Repetitive code (remember DRY, Don’t Repeat Yourself).
- Disagreement between code and specification.
- Off-by-one errors.
- Global variables and other too-large variable scopes.
- Magic numbers.
- Code that could be written more defensively or written to fail faster.
- etc…
- Bad variable or method names.
- Inconsistent indentation.
- Convoluted control flow (if and while statements) that could be simplified.
- Packing too much into one line of code, or too much into one method.
- Failing to comment obscure code.
- Having too many trivial comments that are simply redundant with the code.
- Variables used for more than one purpose.
- etc…
Misunderstandings of the programming language.
Misusing (or failing to use) essential design concepts we have previously discussed in class. (If you’re not sure what some of these concepts mean, you can look them up in Topics and Terms, and we may not have discussed them in class yet.)
- Incomplete or incorrect specification for a method or class.
- Representation exposure for a data abstraction.
- Immutable data types that expose themselves to change.
- Invariants that aren’t really invariant, or aren’t even stated.
- Underdocumented partitions in a testing strategy.
- Underdocumented test case partition coverage.
- Partitions which include illegal inputs.
- Test cases that are not legal clients of the spec.
- etc…
Positive comments are also a good thing. Don’t be afraid to make comments about things you really like, for example:
Process
We will be using a web-based code-reviewing system called Caesar, built here at MIT. Caesar is different from other code review tools (like GitHub, Rietveld, and Review Board) in several ways. It’s designed for reviewing whole programs, rather than changesets. It divides up a program into pieces, assigns them to multiple reviewers, and supports discussion-based reviewing with threaded comments and up and down votes. It automatically ignores widely-repeated code, like boilerplate or library code, in order to focus attention on code that’s worth the reviewers’ time. With this system, you will read small bits of code written by several other students, and many other students will each read a small bit of yours.
Here’s how the process will typically go. Only problem set code will be reviewed. Team projects will not go through this process.
Problem set deadline: first iteration (alpha) of problem set is due.
Morning after deadline: submitted code is loaded into Caesar, and an announcement goes out by email to all reviewers that the reviewing process is open.
If you took slack days on the deadline: you have to wait until the morning after your deadline before you can start code reviewing.
During code review: you should visit Caesar, read the code that you were assigned, and make comments on it. For each piece of code you review, you can:
Make a comment
by clicking on a line of code or selecting a range of lines, and typing your comment.Reply to another comment
by clicking on Reply. You can use this to clarify, expand on, or disagree with comments made by other reviewers.Upvote a comment
by clicking the thumbs-up icon. Use this to agree with a comment made by another reviewer.Downvote a comment
by clicking on the thumbs-down icon. If you’re disagreeing with a human, then it’s polite and helpful to also reply to explain why.
Course staff (TAs, LAs, instructors) will be reviewing the reviews: upvoting, downvoting, replying, and commenting as appropriate.
After code review deadline: Look at the comments on your own code, and start revising your code for your beta submission.
How should I address my code reviews?
Your beta submission must address all the code reviews you received – all the human comments plus all the automated checkstyle comments that are marked #important.
Address each human comment by replying to the comment in Caesar, explaining either how you changed your code to respond to the review, or explaining why the code was not changed.
Address each #important checkstyle comment by eliminating it from your code.
If the checkstyle comment is actually wrong for some reason, you can explain why you couldn’t eliminate it, by writing a comment where it occurs in your source code. But this is very rare — you should be trying to get rid of all the #important problems.
A grader will check your beta submission and deduct points if it hasn’t addressed the code reviews well.
Needless to say, if you didn’t get any code review comments – which sometimes happens – then you don’t have to address any code review comments. Positive comments don’t need to be addressed, either.
How can I see whether I fixed my Checkstyle comments?
You can run ESLint locally on your own laptop using the same rules as the checkstyle code-reviewing bot on Caesar.
Instructions will be provided in the addendum to ps0.
Note that Caesar’s #important tag does not appear in the ESLint messages on your laptop.
Instead, issues that would be #important on Caesar are reported by ESLint tool as errors, and other issues are reported as warnings.
Privacy & visibility
As a reviewer, you are fully identified. The system displays your name and username with every review comment, reply, upvote, or downvote that you were responsible for. All registered users can see your name and what you wrote.
All reviewers, all code, and all comments are visible to registered users. The “all users” link shows a list of all users, and each user has a profile page showing all the comments that they have made. A URL for a chunk of code or a review comment can be viewed by anybody logged in to Caesar, regardless of whether they were assigned as a reviewer for that particular chunk.
Caesar is accessible only to registered users. The system is not open to the net, nor is it indexed by Google. It is only visible to members of MIT courses that have used it.
Respect
Be polite. Sarcasm, insults, and belittling words have no place in a code review. It doesn’t matter whether you’re talking about a person (a fellow reviewer or a code author) or about code. Don’t call code “stupid,” because that transfers all too easily to the author of the code, whether you meant it that way or not.
Be constructive. Don’t just criticize, be helpful: point the way toward solutions. “Hopeless mess” is not a constructive comment; “name the variables more descriptively, e.g. tmp1 is not a great name” is much more constructive.
As a reviewer, you should downvote comments by others that are unconstructive or rude, and serious problems should be referred to the teaching staff.
FAQ for reviewers
Does reviewing affect my grade?
You receive a grade for participating in code reviewing, based on the quantity and quality of the reviewing comments you’ve made.
Does my reviewing affect other people’s grades?
Not directly. Positive or negative code reviews do not factor into the code author’s grade, and upvotes and downvotes do not affect other reviewers’ participation grades. But your reviewing will hopefully help other students become better programmers and acquire a better understanding of the course, and indirectly improve their grades.
How many files do I need to review, and how much do I need to do on each one?
The number you are assigned will depend on the size of the problem set but will not exceed 10 files.
The Caesar system will expect you do to at least one thing on each file assigned to you – make a comment, reply to a comment, upvote, or downvote – after which the system considers that piece of code “already reviewed.”
To earn full participation credit in code review, however, you should make multiple, substantial, specific comments on each file, not just upvote or downvote comments made by others.
I don’t feel like I know anything. How can I possibly review other people’s code?
You know more than you think you do. You can read for clarity, comment on places where the code is confusing, look for problems we read about or talked about in class, etc.
What if I can’t find anything wrong?
You can write specific positive comments that relate good features of the code to ideas we are discussing in the course.
What if I think a Checkstyle comment is wrong or irrelevant?
The Checkstyle bot’s review comments are generated automatically by a style checker called ESLint. Some of these comments might be wrong or inappropriate for the situation, so please use upvoting and downvoting to review Checkstyle too.
How much of the whole program do I have to try to understand?
Caesar presents each file in isolation and asks for comments just for that file. We don’t expect you to look at anything else, or to spend time struggling to figure out whether it’s correct.
How can I find out who wrote this code?
Code authors are anonymous. If there is a serious situation requiring the identity of the code author – e.g. a potential case of plagiarism – then bring it to the attention of the teaching staff.
FAQ for code authors
Can somebody use my code to cheat in the class?
It’s not possible to participate in code review until after you have submitted your problem set (the alpha version).
When revising for the beta, copying code seen during code review would be a serious violation of the collaboration policy. See Problem sets, iteration, and slack days in General Info and Individual work in Collaboration and Sharing.