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.

Your own replies are graded individually, as part of the rubric below. Whether the team as a whole dealt with the review counts toward 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.

GP3: Build and Load

The deliverable is your assigned team's models.py, load.py, any data/ folder, and the new "Data" section of their README.md, described in GP3: Build and Load. This is the first review where you can run the work: check out their branch, run python scripts/load.py --drop, and open planner.db.

Dates. Pull requests are open Monday night, Oct 26. Reviews are due Wednesday, Oct 28 at 11:59 pm. Revisions are due Friday, Oct 30 at 11:59 pm, and I merge over the weekend.

What to look hardest at.

Do the classes match the schema? Put models.py beside schema.dbml and compare one table column by column: names, types, keys, not null, and every unique combination. A difference either way is a finding.
Is the data right? Look at the rows, not just the code. Find a row that breaks a rule the database cannot check, such as a plan that starts before the student was admitted, or an advisor who is not one.
Is the data believable? Ask whether someone in the department would recognize it: realistic proportions, plausible values, and enough rows that their README's questions have interesting answers.
Does the load script follow the rules? Look for an id typed into the code, an unseeded random generator, or anything downloaded while loading.
Is the real data used and explained? Wherever the core provides real data for their tables, check that they use it, and that the README says where it came from and what they left out.

What not to spend your comments on. The design of the tables, which GP2 settled, unless the code shows a table cannot hold what it needs to. Code style that ruff accepts. How they split the work among themselves.

One thing to say out loud in your summary. Answer this question: what is one question from their README that this data cannot answer yet, or answers wrongly? If you cannot find one, say which question you tried and what the data gave you.

Rubric

Each code review is an individual assignment in Canvas worth 5 points. It grades both sides of the review: the comments you leave on the team you are reviewing, and the replies you write on your own team's pull request. Every student is graded on what they submitted under their own account.

Criterion	Points	What earns full credit
Comments	2	Two or more substantive inline comments on the other team's work, including one strength that says why it works and one problem with a fix
Replies	2	A reply to every thread on the parts of your own team's deliverable that you wrote, saying what you changed or why you did not
Etiquette	1	Comments submitted as a review on the Files changed tab within 48 hours, and comments and replies that are specific and respectful

The first two rows carry partial credit.

Comments earns 2 for two or more real comments that include both a strength and a problem with a fix. It earns 1 for a single comment, for comments that say nothing, or for a review that has only one of the two halves. A review that only praises and a review that only criticizes score the same here, and a comment that names a problem without a way to fix it has done half the job. It earns 0 when there are no line comments at all, however good the summary is.

Replies earns 2 when every thread on your part of the deliverable has a reply from you by the revision deadline. It earns 1 when some do, or when the replies say nothing ("ok", "fixed" with no commit behind it). It earns 0 when you replied to nothing. Divide the threads the way you divided the work: the person who wrote the lines a comment is anchored to answers it. If no thread landed on your part, reply to at least one of your teammates' threads with something that adds to the answer.

Etiquette is all or nothing.

Commits can cost up to 2 points. The deliverable is graded as a team, so this is where your own share of it shows. I read the commit history on your team's branch, and you lose 1 point if it has fewer than three commits of your own on at least two different days, and 2 points if it has none. Commits a teammate made on your behalf, or one commit at the deadline holding all of your work, do not count as your share.