How to conduct an effective code review

Submitted ⁨⁨8⁩ ⁨months⁩ ago⁩ by ⁨otto@programming.dev⁩ to ⁨programming@programming.dev⁩

https://optimizedbyotto.com/post/how-to-code-review/

Whether you’re steering an open source project or leading full-time a software development team, the key to maximizing productivity lies in efficient code reviews.

source

Comments

Sort:hotnew top

CameronDev@programming.dev ⁨8⁩ ⁨months⁩ ago
I think a lot of these are opinions stated as facts.

The nitpicking one seems to be using a different definition of “nitpick”. To me, a nitpick is to pick on something entirely meaningless (eg. Fullstops at end of comments, slight incorrect variable names, code alignment). If i see a review full of those I assume the reviewer skipped the correctness checks, and phoned in the review.

The git push --force is definitely a controversial suggestion, im personally happy with doing that, but I have also personally accidentally force pushed dev/main and seen others do it. Squash on merge is probably a safer habit to have. Also, gitlab and bitbucket both get a bit confused if you forcepush to a branch that is part of a MR.

Reviewer fixing problems is also situational. For open source stuff, if you rely on the submitter, youll frequently jusy end up with an abandonned PR. For team stuff, the original author may have already moved on to another ticket, so pushing it back may stretch out the development cycle and cause the code to become stale, and potentially unmergable. Our solution is to just communicate. “This is wrong, I am going to fix and merge. Cool?”

The article is very light on how to actually review for correctness, which in my experience is the thing people struggle with most. Things to look for (Non-exhaustive):

C: Allocations, and deallocations.

Are there leaks in any codepaths?

Are scopes used correctly?

API usage: return values checked? API called correctly? Safe API should be used over unsafe

Thread safety: Are there locks? If yes, focus on these paths, locks are hard to get right. If not, is there anything that should be protected? Some APIs are not threadsafe.

Loops: Are bounds correct, do they terminate correctly.

Comments: Do the match the code? Do they add value? (This is subjective, and down to team preferences)
source
- tatterdemalion@programming.dev ⁨8⁩ ⁨months⁩ ago
  
  The git push --force is definitely a controversial suggestion, im personally happy with doing that, but I have also personally accidentally force pushed dev/main and seen others do it. Squash on merge is probably a safer habit to have. Also, gitlab and bitbucket both get a bit confused if you forcepush to a branch that is part of a MR.
  
  You can add branch protections that will prevent you from accidental force pushing to main or dev.
  
  IMO when I see a PR with “WIP” commits, I just assume that minimal effort was put into keeping the commits organized, and I squash all commits to review the PR. If I see many meaningful commit messages, I will try reviewing one commit at a time.
  
  source
iawia@feddit.nl ⁨8⁩ ⁨months⁩ ago
I’ll just leave this here:

What about code reviews?

source