# 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. Source: https://laszlonemes.com/university/software-technology/code-review · Author: László Nemes Guide · Software Technology 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 read `The review loop` `What to check, in order` `blocking / suggestion / nit` `Answering a review` `MR size` `Reviewer 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? After this session you can - review a merge request in a fixed order, starting from the issue's acceptance criteria - write comments that are specific, marked by weight and about the code, not the person - answer every thread and keep the review history readable - keep merge requests small enough to be reviewed for real - rotate reviewers so everyone knows every part of the game ## 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. > **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. > > *Diagram:* The review loop. The author opens the merge request with the template filled and a reviewer named. The reviewer reads it within a working day and comments, marking each comment blocking, suggestion, question or nit. The author answers with new commits and replies and resolves the threads, and the reviewer looks again. When nothing blocking is left and the pipeline is green, the reviewer approves and the author merges. ## What the reviewer checks In this order – the expensive problems first: | | Question | How to check | | --- | --- | --- | | 1 | Does it do what the issue asks? | Read the acceptance criteria and check each one. For UI changes, check out the branch and try it. | | 2 | Does it break anything else? | Is the pipeline green? Are there tests for the new behaviour? Does it touch code others use? | | 3 | Does 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. | | 4 | Is it understandable? | Names, method length, duplication, comments where the why isn't obvious. | | 5 | Is it tested? | Error paths and edge cases, not only the happy path. | | – | Formatting and style | Not 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 ```text ✗ 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 ```text 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). > **Fig. 2** · Small MRs get real reviews. Large ones get an approval. > > *Diagram:* Review quality falls with the size of the merge request. 50 to 300 changed lines can be read line by line. 300 to 500 lines get thin reading. Above 500 lines the reviewer scrolls instead of reading, and approves without really reviewing. ## 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. > **Fig. 3** · Three people, two directions, alternating. There is no boss: the member who does the release gets reviewed like everyone else. > > *Diagram:* Reviewer rotation in a team of three. In one week A reviews B, B reviews C and C reviews A; the next week the direction turns around. Over a semester everyone reviews every area of the code, and nobody's code goes unreviewed – including the code of whoever does the release. In your assignment **Reviews count** From milestone 2 the instructor looks at who reviewed what. A member who never reviews is missing from half of the process; a member who approves everything in two minutes is visible too. ## What doesn't count as review Common mistake **LGTM after two minutes** "Looks good to me" on a 400-line MR two minutes after the request is not a review. If you don't have time, say that you don't have time. Common mistake **Approving so it's done** A review is only worth something if "no" is a possible answer. Approving to unblock a teammate before a demo moves the bug from the review to the demo. Common mistake **New work inside a review** If the review makes you think of something that isn't part of this issue, open a new issue and link it. The MR stays about its own issue. And never approve your own MR – the setup in the [repository guide](https://laszlonemes.com/university/software-technology/repository-setup) makes the platform refuse it.