loading...

How do you code review?

jonoyeong profile image Jonathan Yeong ・1 min read

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?

Discussion

markdown guide
 

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!

 

Style issues and tests and their coverage should be ran on CI, honestly. It saves so much time.

 

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.

 

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.

 

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.

 

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.

 

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.

 

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.

 

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:

 

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?

 

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.

 

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-...

 

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.)