Guide · Software Technology

Code review: how to write it and how to take it

What a reviewer checks and in which order, how to write comments marked by weight, how to answer a review, how big a merge request should be, and how to rotate reviewers in a team of three.

7 min readThe review loopWhat to check, in orderblocking / suggestion / nitAnswering a reviewMR sizeReviewer rotation

Every merge request in this course is reviewed by a teammate before it is merged. Code review is the most common quality practice in the industry and mandatory almost everywhere; for interns it is one of the first things a team notices – can this person write a review, and take one?

What review is for

Review isn't hunting for someone else's mistakes. It makes the code the team's instead of one person's: two people understand it, two people stand behind it. Bugs found on the way are a bonus. In this course it also shows up in grading – reviews are part of your GitLab activity.

Don't polish to perfection. If the change makes the code better than it was and breaks no rule, it can go in.

The review loop

A merge request goes round the loop until nothing blocking is left (Fig. 1). The platform keeps the score: approvals, open threads, the pipeline.

Author opens the MRtemplate, a named reviewerReviewer readswithin one working dayCommentsblocking / suggestion / nitAuthor answersnew commits, replies, resolveslooks againnothing blocking leftApprovepipeline green, all resolvedAuthor mergesmerge commit, branch deletedThe author resolves each thread after answering; the reviewer reopens it if the answer doesn't convince.
Fig. 1 · Comments go out, commits and replies come back. The author resolves the threads, the reviewer approves, and the author – not the reviewer – merges.

What the reviewer checks

In this order – the expensive problems first:

QuestionHow to check
1Does it do what the issue asks?Read the acceptance criteria and check each one. For UI changes, check out the branch and try it.
2Does it break anything else?Is the pipeline green? Are there tests for the new behaviour? Does it touch code others use?
3Does it fit the architecture?No model class refers to the view; responsibilities sit where the design put them; the wiki is updated if the design changed.
4Is it understandable?Names, method length, duplication, comments where the why isn't obvious.
5Is it tested?Error paths and edge cases, not only the happy path.
–Formatting and styleNot your job – the formatter and the analyser decide. If CI passed it, it is fine.

To try a branch locally: git fetch, then git switch feature/ABC-12-…. The MR page also has a Code → Check out branch hint with the exact commands.

Writing comments

  • Ask, don't order. "What happens if the list is empty?" works better than "This is wrong" – and you may be the one who missed something.
  • Be concrete. Which line, what the problem is, what would be better: "This method is 80 lines; the second half could be a separate calculateCost and tested on its own."
  • Mark the weight with a prefix, so it is clear what blocks the merge.
  • Praise too. If something is well done, say so. It is feedback, not politeness.
  • About the code, not the person. "The code does X", not "you did X".
  • In time. React to a review request within a working day. If you can't, say so, so someone else can take it.
blocking:
Can't be merged without it – a bug, a broken requirement, a missing test.
suggestion:
Would be better this way; the author decides.
question:
I don't understand this – please explain. It may well be right.
nit:
A small thing, if you are there anyway.

The same remarks, written two ways

✗  This is wrong.
✓  question: what happens here if the save folder doesn't exist yet?

✗  Rename this.
✓  suggestion: "process" doesn't say much – "applyMove" would match the use case.

✗  You forgot the tests again.
✓  blocking: the corrupt-file criterion of #12 has no test yet. Could you add one before we merge?

✓  nit: unused import on line 3.
✓  Nice – parsing the map from a string makes these tests very readable.

Receiving a review

  • It is about the code, not about you. The reviewer spends their time making your work better.
  • Answer every thread, even when you agree – "Fixed in a41c9e2." If you disagree, say why; if you can't agree, ask the third team member or the instructor.
  • Push fixes as new commits. Don't rebase during review – the reviewer loses track of what changed since their last look.
  • The author resolves a thread after fixing or answering it. If the reviewer isn't convinced, they reopen it. This way the author holds the list of what is left.
  • If the review finds a lot, that is a signal: the MR was too big. Next time, smaller.

Two replies

Fixed in a41c9e2 – the folder is created on first save, and SaveTest covers it.

Kept as is: "process" is the name used in the spec's state diagram. I added a doc comment
that links them. OK for you?

How big an MR should be

Ideally 50–300 changed lines: one issue, one logical unit. Above about 500 lines a reviewer can't read any more, only scroll (Fig. 2).

changed lines in one MR50–300 linesone issue, read line by line300–500the reading gets thin500+ linesscrolled, not reviewedToo big? Split it: the model and its tests first, then the view.Renames and reformatting go in an MR of their own, so the real change stays visible.
Fig. 2 · Small MRs get real reviews. Large ones get an approval.

Choosing the reviewer in a team of three

The author picks and names the reviewer – and tells them, instead of waiting to be noticed. Rotate: over a semester everyone should see every area of the code (Fig. 3). The person who wrote the related code is a good reviewer, but it doesn't have to be them every time.

Member AMember BMember CA reviews BB reviews CC reviews ASolid: one week's pairs. Dashed: the next week, the other way round.
Fig. 3 · Three people, two directions, alternating. There is no boss: the member who does the release gets reviewed like everyone else.

What doesn't count as review

And never approve your own MR – the setup in the repository guide makes the platform refuse it.