Code Reviews

Every project deliverable is reviewed by another team before I merge it. Reviewing is not a formality that happens after the work; it is part of the work, and it is graded individually.

Most of what you will learn about writing code you will learn from reading someone else's. A reviewer who is not on the team can see the line that made sense to the four people who wrote it and to nobody else. That is the whole point of the exercise, and it is why the reviewing team rotates: by the end of the semester you will have read every other team's work exactly once.

Read GitHub's own overview of giving reviews once before your first review. This page says what is expected on top of that.

Who does what

Who	When	What
Authoring team	Before Tue's class	Completes the assignment and opens the PR when ready for review
Reviewing team	Before Thu's class	Reads the deliverable and leaves comments, one review per student
Authoring team	Before the weekend	Answers the review, pushes follow-up commits, and resolves the threads
Professor	By the next Monday	Reads both sides, approves and merges; leaves feedback for next time

So: nobody in the class approves a pull request. You comment on other people's work and you answer the comments on your own. Approving and merging are mine, because I am the one who has to read both sides before the deliverable becomes part of main.

My approval is the signal that the deliverable is accepted and you can stop editing it. Until it appears, the pull request is still yours — so if you see comments from me and no approval, there is something left to do.

I assign the reviewing teams the morning after the deadline, and GitHub emails you when you are requested. The rotation table says in advance who you will be reading, so you can look at the assignment before you are asked.

A green checkmark is not a review

The automatic check tells you the code imports and the linter is quiet. Every conversation being resolved tells you the threads were closed. Neither one says the deliverable is any good, and I will not merge a pull request the reviewing team has not actually been through.

How to leave a review

Do this in the Files changed tab of the pull request, not in the conversation tab. GitHub's commenting on a pull request has pictures; the short version is:

Open the pull request and click Files changed.
Hover over the line you want to talk about and click the blue + in the left margin. Drag across several lines to comment on a range.
Write the comment and click Start a review for the first one, then Add review comment for the rest. This holds your comments as a draft so the author gets one notification instead of eight.
When you are done, click Finish your review at the top, write a short summary, choose Comment, and submit.

If the + is missing on a Markdown file

A .md file in a diff has two buttons at the right of its header: the source diff and the rich diff, which shows the file the way it will look on GitHub. The rich diff is easier to read and you cannot comment on it. Switch back to the source diff and the + appears. Read in rich, comment in source.

Choose Comment as the review type, not Approve and not Request changes. That holds for everyone in the class and for every deliverable, so there is nothing to remember beyond it. Approve is reserved for me, and it means the deliverable is accepted. Request changes blocks the merge until the same person comes back and dismisses it, which is a bad thing to depend on when that person has an exam on Tuesday. Pull request reviews in GitHub's reference explains the three states if you want the details.

Divide the work with your team before you start. Four reviewers who all read the same opening paragraph have covered one paragraph. Look at the Files changed tab together, see what the pull request actually touches, and say who takes which files — or, when a deliverable is one long file, which sections of it. Then each of you submits your own review. Your review is graded on its own, so do not let one person write "we reviewed this" on behalf of four.

What to write

Each student posts at least two comments on the submitted files. Two is the floor and not the target; a careful review of a three-page document usually runs to four or five.

Across your comments, include at least one of each:

At least one strength. Name the specific thing and say why it works, so the team knows to keep doing it. "Nice job" is not a strength; it identifies nothing they could repeat.

At least one area for improvement, with a way to fix it. This is the part most people get wrong. Do not stop at what is wrong — say what you would do instead. If you cannot suggest a fix, you may not yet understand the problem well enough to raise it, and the honest version of the comment is a question.

A comment that proposes an exact replacement can use a suggested change, which the author can accept with one click. Click the ± icon above the comment box and edit the lines it inserts. Suggestions are ideal for a table name or a rewritten sentence; they are the wrong tool for "this section needs rethinking."

Example comments

Weak

This use case is too vague.

True, maybe, but the team already thinks it is fine, and now they have to guess which part you mean and what would satisfy you.

Strong

"A student views their plan" names an actor and an action, but I cannot tell what the database has to do, so I cannot tell whether your tables support it. Compare it to your fourth bullet, which is specific enough to check. Try naming the state that makes it interesting — something like "a student on the 2024 catalog opens a plan that already has two terms scheduled and sees which of their remaining requirements are unmet."

Weak

Good tables.

Strong

Splitting attempt from enrollment is the right call — it means a repeated course is two rows rather than a mutated one, which is what your third query needs. Worth saying that explicitly in the sentence, since GP2 is where someone will be tempted to merge them back.

Notice that the strong versions point at something particular in this document, and that the improvement carries a remedy.

Professional tone

You are reviewing work, not people, and the people in question are in the room with you on Thursday.

Write about the document: "this section does not say what happens when…", not "you forgot…".
Ask when you are unsure instead of asserting. "Is section the same thing as offering here, or are they different?" is a better comment than a wrong correction.
Assume the thing you do not understand has a reason, and ask for it.
Keep it short. 2–3 sentences per comment is plenty.

Blunt is fine, but dismissive is not. Write the comment you would want to receive.

Responding to a review

When your review comes in, the next 48 hours are yours.

Reply to every thread, even the ones you disagree with. "We thought about that and kept it because…" is a complete and acceptable answer; silence is not. Push your follow-up commits to the same branch, and the pull request updates itself — do not open a second one. GitHub's incorporating feedback in your pull request covers replying, committing suggestions, and resolving.

main requires every conversation to be resolved before it will merge, so work through the threads until the count at the top of the page reads zero. Resolve a thread once you have replied and pushed whatever you agreed to. If a reviewer thinks you resolved something you did not address, they should reopen it and say so.

How you respond is part of your team's grade on the deliverable. A team that took three of five suggestions and explained the other two has done this correctly.

Knowing when you are done

Your team's part is finished when three things are true, and none of them involves clicking Approve.

Every thread has a reply.
Every follow-up commit you agreed to is pushed to the branch.
The unresolved conversation count at the top of the page reads zero.

Then leave the pull request alone. Do not close it, do not open a second one, and do not merge it — you could not merge it if you tried, since main accepts pushes only from me.

I read the deliverable and the review after that, and one of two things happens. If it is ready, I approve, which is my way of saying the work is accepted and nothing further is needed. If it is not, you get a comment from me saying what is missing, and the branch stays open until you deal with it.

Watch for the approval, not the checkmark

The green check is automatic and arrives minutes after you push. The approval is me, and it arrives after I have read what you wrote. That one is the one that means you are done.

Instructions for each deliverable

The general expectations above apply every time. Each deliverable adds a short list of what to look hardest at.

GP1: Requirements

The deliverable is your assigned team's README.md, described in GP1: Requirements. It is prose, not code, so you are reviewing whether a database could be built from it.

Dates. Pull requests are open Monday night, Sep 21. Reviews are due Wednesday, Sep 23 at 11:59 pm, so both teams can read them before Thursday's class. Revisions are due Friday, Sep 25 at 11:59 pm, and I merge over the weekend.

What to look hardest at.

Are the use cases specific enough to check? A use case you cannot imagine testing is a category, not a use case. Point at one that is and one that is not, so the contrast does the teaching.
Could the proposed tables actually answer the proposed queries? Pick one of their analytical questions and try to trace it across their table list. If you get stuck, say exactly where — that is the single most useful comment you can leave on this assignment.
Does anything in the document belong to your area instead of theirs? You are the team most likely to notice, and an overlap found now is a conversation, while the same overlap found in October is a schema change.
Are the boundaries and open questions real? A team that lists no assumptions has not looked hard enough. If their boundaries section says they need something from you, say whether you plan to provide it.

What not to spend your comments on. Column names, keys, and types are GP2, and the assignment tells them not to specify those yet. Do not review word count, and do not proofread — a typo is not worth one of your comments unless it changes the meaning.

One thing to say out loud in your summary. Answer this question: if you had to build a database from this document tomorrow, what is the first thing you would have to go ask them? That question is the review.

GP2: Schema Design

The deliverable is your assigned team's schema.dbml and schema.png, plus two edits to their README.md, described in GP2: Schema Design. Comment on the .dbml, since you cannot anchor a comment to a picture, but read the .dbml with the .png open beside the file.

Dates. Pull requests are open Monday night, Oct 5. Fall break stretches the usual clock. Reviews are due Monday, Oct 12 at 11:59 pm. Revisions are due Wednesday, Oct 14 at 11:59 pm, and I merge before GP3 starts.

What to look hardest at.

Trace one of their queries through the schema. The README now lists the tables each query reads. Pick an analytical one and follow the query column by column: which join, on which key, filtered by which column. If you get stuck, say exactly where, as you did in GP1.
Does each primary key match the table's note? Read "one row is one …" and then read the key. Name one row the key would reject that the domain allows, or two rows the key would allow that the domain forbids. Either one is a strong comment.
What is nullable that should not be, and what is stored twice? Look for a column without not null and no note saying when the value is missing, a stored value a query could compute, and a copy of something the core already holds.
Does anything cross into your area? You are the team most likely to notice a table that duplicates one of yours, or a reference that should not exist. Check their table names against yours as well, since both will live in one database.
Does the picture match the file? A table or a line in one and not the other is worth a comment.

What not to spend your comments on. Naming style, unless a name is misleading. Layout, unless the diagram is unreadable. Type choices between two that both work, such as integer and bigint.

One thing to say out loud in your summary. Answer this question: what is one situation from their use cases that this schema cannot store, or one fact the schema can store twice? If you cannot find one, say what you tried.

Rubric

Each code review is an individual assignment in Canvas worth 5 points. Every student on the reviewing team is graded on the review they submitted under their own account.

Criterion	Points	What earns full credit
Coverage	2	Two or more substantive inline comments, anchored to specific lines, spread across the work under review
Balance	2	One comment names a strength and says why it works; another names a problem and gives a way to fix it
Etiquette	1	Submitted as a review on the Files changed tab, within 48 hours, specific and respectful

The first two rows carry partial credit.

Coverage earns 2 for two or more real comments, 1 for a single comment or for two that say nothing, and 0 when there are no line comments at all, however good the summary is.

Balance earns 2 when both halves are there, 1 when only one is, and 0 when the review is neither. A review that only praises and a review that only criticizes score the same here. Note that the improvement half is not satisfied by naming the problem alone; a comment that stops at what is wrong has done half the job.

Etiquette is all or nothing.

Responding to the review you received is not a separate Canvas assignment. It counts toward your team's grade on the deliverable itself.