Mistakes I see engineers making in their code reviews
38 points by untitaker
38 points by untitaker
The author seems to take Google's senior principle out of context a little bit. Here is the full quote from their guidelines:
In general, reviewers should favor approving a CL once it is in a state where it definitely improves the overall code health of the system being worked on, even if the CL isn’t perfect.
That is the senior principle among all of the code review guidelines.
I think "favor approvals once the overall code improves the general health of the system" and "bias toward favoring approvals" is very different with the former being very reasonable to me.
That bit also stuck out to me as something that isn't true and showed a clear bias of the author. More than the other points anyway, as most of those did seem quite reasonable to me.
I am probably a bit more conservative than Google there. To be frank, given the state of some Google products they could be a bit stricter as well, but that is a bit of a tangent I suppose.
Perfect shouldn't stand in the way of good enough. But good enough also shouldn't come with an amount of tech debt that will be annoying to clean up or cause issues down the line.
I am also not working on SaaS products, the author explicitly mentions SaaS features as a specific example. On a personal level I don't think that shouldn't matter, but realistically it is clear that the bar is somewhat lower somehow for SaaS products features and products compared to other software.
But good enough also shouldn't come with an amount of tech debt that will be annoying to clean up or cause issues down the line.
That’s a situational question. Neither position is universally correct
Friction between a PR author and its reviewer is most common when the PR author is an application developer and the reviewer is a maintainer of a shared library or core API. Even if it’s strictly internal, its maintainers are responsible for looking out for the needs of other applications that use it and for its overall roadmap. I’ve been on both sides of this and agree that this situation needs a lot more nuance than “too much gatekeeping” and approval by default. Asking whether your library/API is SQLite is not enough. I’ve found that, however much time it takes or uncomfortable it might be, it’s usually better to discuss such changes before opening the PR.
Some good advice, but a few things I disagree with:
What do you do when there are twenty places in the diff that you’d like to see updated - for instance, twenty instances of camelCase variables instead of snake_case?
Code review should not have these comments, they are mechanically checkable. Formatting things should not be part of code review. Run your language’s equivalent of clang-format and clang-tidy in CI.
The kinds of naming convention things that should be in code review are rules about making methods verbs, noun-adjective naming order for types, avoiding abbreviations, and so on that can’t be mechanically checked.
Don’t review with a “how would I write it?” filter
I think this advice can lead to losing some of the benefits of code review. Asking how I would write it first leads to a few positive things:
Review with a “will this work” filter, not with a “is this exactly how I would have done it” filter
I really dislike when code review is expected to catch the ‘will this work?’ bit. That’s what tests are for. The code review then focuses on which test cases are missing. Some things (subtle concurrency issues, for example) may be too hard to test and then ‘will it work?’ reviews are unavoidable, but they shouldn’t be the norm. The code review is about ‘can we maintain this long term?’ or ‘does this introduce subtle problems for something else that we are planning to add?’.
I agree on your stance to "Don’t review with a “how would I write it?” filter".
There's basically two flavours of that:
Overall, my goal with reviews is not only making the codebase better but also doing some teaching so the overall quality of the team rises.
I also do want to get really critical comments, as you can always learn from mistakes and other ways to do things.
(3) is more legitimate than you give it credit for - often there are two or more approaches that are just fine for the use case and the trade-offs are hypotheticals. Maybe one of them is more self-contained (often good) but the other will be more obvious for keeping it in sync if adjacent code changes in future (often good). Developers will tend to value different hypotheticals more and it’s healthy to identify it quickly and let the author move on without unnecessary fuss.
Edit: Maybe this example is just a subcategory of (2) but I think it’s hard to draw a line between “more elegant” and subjective style preferences since the latter are usually rooted in a legitimate quest for better code, even if it’s hard to prove the value in the here and now.
I think the distinction between 2 and 3 is that 2 will make the code more readable or more concise without actually bringing technical mertis and 3 would be more of a "i would have ordered the fields differently".
i do consider:
so the option 3) should be avoided at all cost.
but this is all pretty subjective, so it's always good to talk to your colleagues and co-contributors about this.
Sean's writing is top-tier stuff. He's been in my short list of favorite blogs lately. I deeply connect with his points of bringing system-awareness and consistency to reviews, while still biasing toward unblocking.
It's worth noting that some of these change depending on the stage of company, though. It appears to be harder to have as high of a bar when trying to survive and find product-market fit. That's far from my expertise, but it aligns with disagreements I've had in the past. :)
Recently I have been working with more academics, and many of them have never worked in an environment that does code review. I've been trying to get more people to do code review and have been surprised by how often the process breaks down, and two people end up working on different branches that they can no longer reconcile. Either a reviewer is too nitpicky, submitters can't get any of their code merged, and the submitter starts using a branch instead, or a reviewer is too accepting, code goes into main that the reviewer doesn't really want to use, and the reviewer starts using a branch instead.
I wish there were a guide to "code review for small teams of two or three people where nobody has ever done code review before" that I could point people to.
It is my experience as well. I think the answer is the usual: writing documentation, presentation and pair programming. All very time consuming and you need buy in from management as well.
The whole idea of building software by approving or rejecting tiny change sets is a rather silly one. Where does the architecture gets established? By whom? Why do tiny incremental changes go through all this process but not the architecture itself?
I think the points the article makes are a consequence of this reality. To a great extent, itself a consequence of the whole Agile dogmas.
Why do tiny incremental changes go through all this process but not the architecture itself?
I don’t see that this follows. In systems I’ve worked onl both go through review, but different kinds. The architecture goes through a cycle of design review and then initial implementations are also reviewed. The review process for changes is usually different, but that doesn’t mean that the review process for the original system didn’t exist.
In my experience (and it's with only one company I left because it got so insane) is a company with software no one there truly understands (most, if not all, the original developers have left; those were the ones who established the original architecture) and yet the software still makes money. Code does require maintenance as the environment changes, so what's left? Doing as little as possible to not upset the money press.
Adding on to the author’s opinion about less gatekeeping and that most reviews shouldn’t be blocking: many times I will suggest an improvement with the disclaimer “this could be in a followup PR”. Once in a blue moon, when I think the feedback is subtle or difficult to communicate I will write a PR on top of the PR branch. At times I will commit directly into the original review branch for something like a typo fix, where a feedback loop seems unnecessary. Definitely need to be careful about this because some workflows include force pushing.
There’s a lot of subtleties there, and I’ll just say I agree with the author’s distillation.
Are there any good tools that actually enable/encourage the review of code that isn't just the diff? Github etc only shows you the diff and it's pretty difficult to follow code outside of the diff without leaving the review interface.
This is one reason why I make PRs that consist predominantly of extremely small commits (and I mean extremely). If I have a PR of, say, ten commits, typically nine of them will be trivial refactorings that are trivial to assess as correct without further context, and one will be a "payload commit" that itself can be very small (thanks to the supporting refactorings) and itself is straightforward to see as correct without much additional context. An example is a PR I made to Opaleye, my Haskell Postgres ORM. The content is actually due to Shane O'Brien, but I manually split it into these tiny commits so I could more easily see it was correct.
I haven't been very successful in persuading others of the benefits of this approach though!
There is this VS Code extension for PR reviews: GitHub Pull Requests - but the comments say that you can't build or go to definition. But it does show the whole file at least. I haven't tried the extension but I think I'll give it a go at work this week.
I ended up with a Bash script to create a worktree checking out the PR branch then soft resetting it to the PR base to surface the diff as part of the IDE's in-built Git UI as uncommitted changes. Not the most streamlined solution but it lets you treat PR reviews as separate from your own work without having to muck around with stashes and branches, while retaining all the IDE features for navigating context.
Do you have that bash script shared anywhere. That sounds like a great solution!
Here you go: https://gist.github.com/msfjarvis/721e29b2f29654ef185057e3a8f5b409
It's reliant on the gh CLI since I use GitHub for work but shouldn't be impossible to replace the uses for other forges.
Figure out how to pull the code down to review as easily as possible. It’s the best solution.
Web based code view is inherently flawed no matter how hard code forges try to build an iDE in the browser
Yeah, this is the way. I was just wondering if there is a recommend way to do this or if anyone has worked out a way that works well for them.
IntelliJ IDEA, including the IDEs for other languages that are based on it, does a decent job of this. You can check out the PR branch and the IDE knows it's a PR (meaning it has UI to leave comments and approve/reject) but also you get all the regular code navigation features you'd get with any other branch.
I've been (very slowly) working on rolling my own review platform and this is one of the two main features I'm aiming for - along with supporting some of the code-navigation tools you'd expect from an IDE
Code review approach is also heavily dependent on the guy who wrote the code. Imagine these colleagues:
All great points imo. One case I haven't seen mentioned is when there's no consistent patterns in the codebase yet (small teams, very different profiles and/or experience levels). Ofc overall data access pattern, API interface, logging, testing,... Should all be agreed upon earlier in the design process. But what if there isn't any and each dev does whatever they want? I found that the code review process is maybe a good time to "find out" in what direction the codebase should go but it'll definitely increase the number of stalled PRs and blocking reviews.
I haven't found a better way and it's hard to have everyone sit down and agree on a direction when parts of the team might not know what that direction looks like. It also puts a lot of pressure on more senior devs if they need to review all PRs at first. It also comes back to gatekeeping and sometimes awkward conversations.
I'm curious if anyone has had this experience?
There aren't really many templates for these situations because every company that has this problem has a distinct culture, distinct way that their code is broken and inconsistent, and also sometimes the "inertia" that keeps the codebase and engineering practices this way is different per org.
I generally agree with you that a lot of this advice is for "good weather" where you have the capability/power to enforce best practices.
The only one-size-fits-all, but undoubtedly heavy-handed pattern I've seen here is rewrite code entirely and split it out into sattelite services that can be owned by individual teams. It's easier to enforce best practices in small teams than for the entire engineering org. But again, very heavy-handed, and an inversion of conway's law that sometimes leads to bad architecture choices.