: Methods for writing a code review I currently spend a good deal of time over at Code Review and I would love to improve the quality of the reviews I write. Can you give me any insight into

We do a code review for a quality measure. I typically do code reviews via a two-step approach: (1) do a light pass e.g. checking necessary syntactical, style and standard rules across the team; (2) a contextual pass for a fine-grained detailed review e.g. reviewing context, patterns and test coverages. For your reference, I created a code-review checklist sometime ago and posted it on my Github account.

---

(0)

The type of code review written in a professional programming project is a completely different animal from what is on CR SE.

The reviews on CR SE at least try to follow the general SE guideline of "be nice". They also often suggest alternative ways to fix the problem, and argue that one fix might be "better" than another.

Professional code reviews don't waste words on "being nice", and their purpose is not to fix errors, but only to find them. In a programming team, everybody knows that everybody (including themselves) makes mistakes. The purpose of code review is to find them, not to avoid bruising delicate egos.

Much of the content of CR SE reviews would (or should) never get as far as a professional code review. Issues like inappropriate use of "namespace" etc should be pickup up by software, not by humans - and in any case, you wouldn't expect a professional programmer to break the basic coding standards that apply to the project he/she is working on. Much of the code being reviewed on CR SE isn't written to follow any particular standards, except the nebulous concept of "good practice".

A professional review might well say something as terse as "Line 75: off by one error" or "Line 80: fails when foo() returns a null value." If the best way to fix the problems is to completely refactor 10 or 20 functions in the code, it's not the job of the reviewer to decide that is the best solution, or to do the work. But that style of writing won't get you many up-votes on CR SE!

---

(0)

It depends on the context

Code reviews can be done for various reasons and the way to write one it heavily dependent on the purpose. Some of the reasons you might write a code review:

As part of a Software Quality Process

As a formal deliverable to management or customers
As quality assurance measure

As an answer on Code Review Stack Exchange
As an academic assessment or grading of one
As part of a hiring process for applicant submitted code

There are a bunch more but those are some of the more common ones. The first point are the kind found most commonly in industry. I don't have experience in all of them and I won't pretend to be an expert on writing them. However here is my advice.

Writing as a formal deliverable

When you are writing a code review as a formal deliverable it is the same as any other report you would write. This should be a version controlled document with formal headers, footers, table of contents, etc... The goal of this kind of review is to prove that best practices have been followed and the product mets company standards. This document will be archived and kept for a long time.

I don't know of any publicly available structure for this kind of document but most companies will have their own format that needs to be followed. In general you are looking for:

Code formatting
Error handling
Quality and number of testcases
Adherence to best practice or company standards
Quality of documentation

Writing as a quality assurance measure

The most common situation for this type of review is on change-requests to a code base. Other developers will review the code to assess if it should be merged into the main branch. I have always conducted these reviews through tools such as Github or Bitbucket. These tools provide line by line comparisons to the existing codebase and allow you to attach comments directly to the code in question.

The style for these reviews is less of a formal review but more similar to a social media post. You can comment on anything you think needs to be changed. "Why did you do this?", "You missed a const here", "Rename this variable to X", and "Please add documentation to this function, I don't understand it" are all examples of the kind of comment you would expect in these reviews.

These reviews are transient in nature as the developer can upload fixes to your concerns and also post replies to explain things. It is about ensuring only good code gets merged, not about writing an amazing report.

Writing for CR.SE

I have no expertise in this area so I will just link to Code Review's help centre on How to Write a Good Answer. The advice there can be summarised to; be insightful, organise your thought, do the best job you can and be polite.

Writing for academia

Most homework assignments will fall into a very similar category to 'Writing as a formal deliverable' and you should follow the same advice. When conducting a code review as part of a marking exercise you will usually have the 'correct' answer with you. It is likely no one else will ever see this review unless their marks get audited, write it however you like so long as you give a fair assessment.

Writing as part of a hiring process

This is similar to writing a review as part of a marking process. One key different is that you are specifically looking for skills the developer is strong in and areas where they need work. Your review should attempt to assess the skill level and experience of the applicant. This will be shared with the hiring manager and/or other members of the hiring panel. Keep it professional in case is it ever seen by anyone else.

General Tips

These tips apply to all types of reviews.

Only comment on inconsistent code formatting. Do not bother with personal style unless you are working on the same codebase and have a style guide. Tabs vs Spaces isn't an argument worth having on every review.
Look for bugs, typos and edge cases. Give detailed comments anytime you find an actual execution problem with the code. Potentially suggest a solution if you can identify it.
Back up your critiques with references. Any time you call out something that is bad practice but functional you should provide a reference to why it isn't optimal. This isn't about your opinion vs theirs so you only want to suggest actual meaningful improvements. These will have articles or guidelines that you can references in your critique.
Don't write their code for them. Sometimes is can be tempting to suggest a complete change just because it is not the way you would have done it. That isn't the purpose of a code review. If there are good reasons to change the approach you should explain them but this is their code. Let them solve the problem their way, point out flaws but don't attempt to solve the problem and explain it to them until their code matches yours.

---

(0)

How you structure a code review depends on the tools you're using and the level of scrutiny that was requested. Instead of giving you an exact template, therefore, I'll address the different types of content.

At the lowest level, a code review can include feedback on individual lines or sections of code. Most important is to point out any errors you find; you might also point out undesired consequences (side effects, performance problems, etc). Think of this as the fine-toothed-comb level.

At the next level up, you might comment on some common themes -- concurrency, asynchronicity, pounding the snot out of the server, whatever. These are the kinds of issues you want to talk about broadly instead of pointing out every single place in the code where it happens, so you need to both explain the problem and give the review-ee enough information to be able to find and fix the individual occurrences.

At the highest level, you might have feedback not about the code itself but the design or architectural approach. Ideally there won't be too many of these because you'll have had design reviews earlier on, but sometimes stuff comes up. In deciding how to provide feedback here, ask yourself: how likely is it that anybody can actually act on what I'm going to say? Sure, this would have been better if you'd used a completely different framework or made these fundamental design decisions differently, but if that's not going to change now, there's no point in spending time on it. The purpose of a code review is to improve that code, not wish that completely different code had been written. That said, if there is an opportunity to advance (rather than ditching things and starting over), you probably do want to point that out -- e.g. the upcoming version of this library you're using is going to have feature X, which we could use to simplify this part of your code.

Now, how to present it? The lowest level is best done with some sort of code markup, like that provided by code-review tools. (I'm thinking of, for example, the git diff on pull requests here.) The other feedback, the stuff not tied so deeply into the code, is in my experience best done in a single higher-level document -- which could just be a text file -- that accompanies the code-level comments. This higher-level document might be anywhere from a few paragraphs to several pages, depending on the amount and importance of the code, the magnitude of the issues, and the level of detail requested. Try to establish some shared expectations about this beforehand; getting a tome when you expected a handful of comments or the reverse is going to be frustrating for the recipient.

---

(0)