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. Some companies like Google have instituted review-before-commit as a required policy. You can’t get a line of code into the Google source code repository unless another Google engineer has read it, given feedback about it, and signed off on it.
The benefits of code review in practice are several. 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 by explicit feedback and by example. So code reviewing is not only a practically important skill that you will need in the real world, but also a learning opportunity.
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, easy to understand, and ready for change. 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.
- Optimistic, undefensive programming.
- Magic numbers.
- 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.
- Misuse of
- Inadvertent use of integer division instead of floating-point division.
- Use of a fixed-size array where a variable-length
Listwould be more appropriate.
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 datatypes that expose themselves to change.
- Invariants that aren’t really invariant, or aren’t even stated.
- Failure to implement
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.
Upvote a comment
by clicking the thumbs-up icon. Use this to agree with a comment made by another reviewer. Some comments in the system are made automatically by the Checkstyle style checker, and it’s particularly useful for human reviewers to upvote or downvote its comments as appropriate.
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. You can address a code review either by changing your code to reflect the review, or by replying to the review (with a source code comment or a reply in Caesar) to explain why the code was not changed. A grader will check your beta submission and deduct points if it hasn’t addressed the code reviews.
The 6.031 Eclipse setup installs a Checkstyle plugin into your Eclipse, configured with the same rules as the checkstyle code-reviewing bot on Caesar.
You can run Checkstyle once by right-clicking on the project and selecting Checkstyle → Check Code with Checkstyle. Its warnings appear in the Problems tab, just like compiler warnings. But if you run it this way, Checkstyle will not update its warnings as you edit your code; you will have to manually run it again.
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.
As a reviewer in Caesar, you have a reputation score, shown in parentheses after your name. You earn 1 reputation point for each upvote (by another user) of a comment you wrote. If you start reviewing early, then you are more likely to get upvotes. If you write good comments, then you are more likely to get upvotes.
As a student in 6.031, your numeric reputation score has no effect whatsoever on your grade. You do receive a grade for participating in code reviewing, but the reputation score is irrelevant to that grade. Instead, we will base your code review grade on the quantity and quality of the actual reviewing that you’ve done, by looking at the activity history on your profile page.
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, but 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.
Yes, it contributes to your participation grade; see the Reputation section above.
Not directly. Problem set grades do not take code reviewing into account, and other reviewers’ participation grades are not affected by your upvotes or downvotes. But your reviewing will hopefully help other students become better programmers and acquire a better understanding of the course, and indirectly improve their grades.
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.”
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.
You can write a specific positive comment about something good.
They are generated automatically by a style checker called Checkstyle. Some of these comments might be wrong or inappropriate for the situation, so please use upvoting and downvoting to review Checkstyle too.
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.
Code authors are anonymous until they reveal themselves – e.g. by replying to a comment and stating that they’re the code author. You can write a comment asking a code author to reveal their identity, or contact you, if you wish. 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.
Since code reviewing doesn’t start until after the alpha deadline, this would be a problem only for students with special extensions. Those students don’t participate in code review.
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
Yes, if you want to reply to a comment on your code or directly contact one of your reviewers, you can do so.