DEV Community

How do you code review?

Jonathan Yeong on June 21, 2020

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 revi...
Collapse
 
sirseanofloxley profile image
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!

Collapse
 
ben profile image
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.

Collapse
 
loujaybee profile image
Lou (πŸš€ Open Up The Cloud ☁️) • Edited

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.

Collapse
 
jonoyeong profile image
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.

Collapse
 
artis3n profile image
Ari Kalfus • Edited

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.

Collapse
 
sirseanofloxley profile image
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.

Collapse
 
sandordargo profile image
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:

Collapse
 
jeriel profile image
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?

Collapse
 
evertones profile image
Everton Schneider • Edited

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
Collapse
 
eljayadobe profile image
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.

Collapse
 
circleofconfusion profile image
Shane Knudsen

I'll be interested to see other responses!

One thing that did work for us at my old job was to modify the pull request template in GitLab to have a short list of questions that the user should have good answers to before submitting.

Some of it was as general as, "Did I write documentation?" or "Did I write tests?", and some as specific as, "Did I use one of the deprecated functions in new code?" (We had some ill-conceived abstractions early on, didn't have the time to clean them all up, but didn't want to pile on more technical debt.)

We mainly wanted to head off some of the non-negotiable stuff that were hard requirements of the contract. Better to catch yourself not writing some unit tests now, than attempt to write hundreds of them months from now.

For me, I found that I'd spend that extra bit of time making my pull request look good, knowing that I'd encouraged people to give an honest review.

Collapse
 
gsandec profile image
Gustavo Silva • Edited

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.
Collapse
 
bytebodger profile image
Adam Nathaniel Davis

I think your post is focused more on the tactical details of how PR's are accomplished, but I just wanted to share that the whole process of PRs engenders enough frustration in me that I felt compelled to write an entire post about it not-too-long ago:

dev.to/bytebodger/the-contentious-...

Collapse
 
tbroyer profile image
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.)

Collapse
 
alexandrudanpop profile image
Alexandru-Dan Pop