How do you code review?

At some point in your developer career you'll have to do a code review. It seems everyone does it differently. I spend about an hour on a code review. Any more and my brain is toast. I also have a checklist that I run through:

• Are there any style issues?
• Does the code placement make sense?
• Is one method doing too much?
• Are there tests? If not, should there be tests?

How do you perform code reviews? What's worked well for you and what hasn't?

Comments

[Sean Allin Newell]

My suggestion for a thorough code review of any given PR/MR would be:

• read the ticket if any (get as much context before reviewing)
• read through the commit messages (this is the code's story)
• pull the code locally and run the new code
• run the tests
• look at the full diff quickly
• look at each file changed carefully
• ask yourself these questions:
  • can this PR be broken up into smaller ones?
  • how can this be made more maintainable?
  • how can we increase test value/coverage?
  • are there any edge cases?
  • did we forget any requirements?
  • does this make the system harder to run locally?
• call out things in the PR that are praiseworthy #encouragement
• bring others into the PR to brag on what you see or call in extra help/experts.

Some of this can be done quickly for small PRs, other times you may need to do 5-15min spurts of review as you have time.

Then approve and merge that sucker in!

[Ben Halpern]

I'm not particularly structured in my approach, so you'll get more relevant answers from others, but here's what runs through my head....

In my particular role of being fairly big-picture-oriented, I tend to ask whether the approach taken is in compatible with future needs—Tests certainly play a big role in this, but asking whether the next thing built with this new code as a dependency will work is important.

Sometimes even the worst code style is perfectly fine if it's hidden behind the right interface and it's not going to result in any major hidden gotchas after we forget about the problem being created.

[Lou (🚀 Open Up The Cloud ☁️)]

Sometimes even the worst code style is perfectly fine if it's hidden behind the right interface and it's not going to result in any major hidden gotchas after we forget about the problem being created.

Ah totally agree with this.

It's something I always referred to as: problems nearer the "leaves" of an application can be more lenient, but those closer they to the "trunk" of the application, require greater strictness.

[Jonathan Yeong]

This is awesome advice! I've always treated code reviews in isolation, definitely going to start thinking about the big picture in my next reviews.

[Ari Kalfus]

Outsource all style issues to a linter that runs in CI. You don't need to spend brain power worrying about bracket placement.

Aside from any checks of the complexity and maintainability of the code, the PR itself should have some context as to what problem it is solving. I recently rejected a one-line change PR because it was setting an environment variable and there was no context in the ticket or PR about why this was being set or what problem it was solving.

Each PR needs to strive for greater maintainability of the code. They need to tell a story about what is being solved. Without exception, any PR that isn't solving a problem is a waste of time. "Updating README" vs. "Updating README to make it more clear how upstream libraries interact with this API." Now there's a story, there is context for the code changes.

[Sean Allin Newell]

Yes, your review will be much more valuable the more robots can do.

I think we should occasionally evaluate the robots and see what you can add to the linter/tests or take away too though; and no better time than code review to do that.

[Jeriel Ng]

Couple of things I usually look out for:

• Could the code be condensed for readability?
• Should the code be broken into separate functions/classes?
• Do the function and property placements comply with the architecture?
• Are there any obvious crash scenarios that could be protected?
• Are function/property names clear and concise?

From a personal standpoint, I have an additional question: how deeply should we be looking out for business logic here, or should that be done during manual testing/QA?

[Everton Schneider]

Many good things have been addressed in the comments. A few things I usually take into account when reviewing are below.

For the code:

• understand the context of the changes
• make sure the scope of each commit is correct (it makes it easier to revert, change the content or drop the commit, if wanted)
• make sure the naming and the design are consistent with the existing architecture
• make sure there aren’t duplicated code that could be encapsulated in a centralised method
• make sure the visibility of the variables and methods are as strict as they must be (private, protected, public)
• identify and address possible enhancements even in code, even if that’s not changed in the code under review, if that proves to improve the codebase (The boy scout rule)
• identify comments that could be replaced by well-named methods that implement the logic
• request to remove useless comments (e.g. a method save() with a comment “save the entity”).
• search flag parameters (booleans) and try to eliminate them
• make sure the scope of changes or the logic in the class method belong where they are - they may be part of some other component in the architecture
• avoid the use of null (in Scala I try to replace them all with None or Optional)
• there aren’t TODOs in the code not addressed with a ticket to be worked on later

For the ticket:

• the title and description of the ticket are easy to understand by someone that hasn’t worked on it
• all automated tests have passed
• there is a well-written test plan

[Sandor Dargo]

If by style, you mean code formatting, try to automate it. I mean the formatting. In the long run, it takes less time than fighting over it.

In a code review, probably the most important thing is to make sure that it is always about the code and never about the person. If you have that, you are on the good path to meaningful discussions.

Hopefully other people in the team ask themselves the same questions you asked and replies should be noted and probably they should be part of the code review guidelines in your teams.

I elaborated a bit more on this topic in a post, you might find it interesting:

Sane office environment with code review guidelines

Sandor Dargo ・ Mar 28 '18 ・ 6 min read

#guidelines #codereviews #rules #efficiency

[Eljay-Adobe]

If I'm very familiar with the code in question, I approach the changed code in the review with a critical eye with how it changes the behavior.

If I'm not familiar with the code in question, I do what you listed: style issues, code placement, lament over functions that do way too much. (We basically don't have tests. But, yes, there should be tests. My code base has plenty of awful non-linear functions that are between 10,000 and 100,000 lines long, with cyclomatic complexity that can only be properly expressed with transcendental numbers.)

Management believes that code reviews will transmit subject matter expertise through osmosis. But in my experience, code reviews do nothing of the sort except give management a warm fuzzy that they are facilitating and encouraging knowledge transfer.

To get knowledge transfer, you either need to be in a mentor/mentee situation or pair programming. Both of which also entail continuous code reviews.

Also, if the code review is small, I can assess it well. But as it gets larger, it becomes difficult to see problems except at a gloss level. I saw one tool (from Google, iirc) size code reviews as wee (1), tiny (<5), small (<30), medium (<100), nominal (<300), large (<1000), freakin' huge or mondrian (<2000), jupiterian or jovian (<3000), month long (<4000), whopping big (<5000), mother of all code reviews (<10000), grandmother of all code reviews (<20000), category 5 code review (anything larger).

Where "gloss" are things like suggesting better named identifiers, misplaced tabs if the team convention is spaces (or vice versa), misplaced curly braces from the team convention, suggesting a breadcrumb comment for some confusing logic. Those are basically superficial things.

[Thomas Broyer]

Automate everything that you can, then code reviews can focus on code design, complexity, maintainability, etc.
And, very important but too often overlooked, look for what's not in review (shouldn't that other file be changed too? why did you change this and not that? You're adding an implicit relationship with some other part of the code --e.g. a mail template links to a page of the app without using a shared router to build the URL, possibly because the router is clientside in the SPA--, then please add comments both sides so we can remember to change the other in case we change one, etc.)

[Gustavo Silva]

I decided to repost here an article that helped a few developers in the past.

This is related to Pull Requests but it may add value to some of you too!

A few tips:

• Common pitfall: do not review a PR with the author
• Your comments should be directed at the code, never the author.
• Optimize for readability, because that is where you and your teammates will spend most of your time.