How to run a miserable code review
badsoftwareadvice.substack.com
badsoftwareadvice.substack.com
A code review should never be done in person; instead all communication should happen asynchronously through passive aggressive messages left in GitHub. If you're on bi-weekly sprints, the review should be left to age for a minimum of 5 business days before any feedback is provided. Never write resolvable comments, but rather leave ambiguous musings of how a block of code could be cleaner. Assign the review to multiple engineers but never make it a formal part of their responsibilities. On special occasions, unpredictably merge a review with no feedback, or do so simply based on your mood. Loosely, and unpredictably, enforce documentation requirements. Most important: never update your product managers on merged features, so that they can experience the joy of discovery just like your customers!
Add an ellipsis to really drive home that only will any more explanations not be forthcoming but it should be evident that this is my problem...
I am extremely ashamed to say I have left a comment like this before. At the time, I was frustrated about never being able to discuss anything over a call. There is no excuse, though.
Often all I want is for my team to approach similar problems differently in the future, not necessarily to refactor the immediate code. Or at the very least consider alternative approaches
Although I much prefer to add automatic code formatting and linting and stuff to codebases to dramatically reduce the occurrence of 'nit:'
This way the value of the pr process is captured: preventing obviously bad changes from reaching prod and two-way knowledge sharing.
Setting a higher bar for reviews more often than not blocks people for days without good reason.
(It’s not like I care either, the project sucks anyway.)
However, yea, against just approving your own code, this is better.
Like man it's code, there's an infinite amount of "could do" with an infinite amount of contexts.
I found google's advice to be pretty good https://google.github.io/eng-practices/review/ while they give a lot of good advice / suggestions, they also make a point that there aren't a lot of hard stops and generally if the code works and isn't horrendous you let it go.
I think a nice little "Hey I saw this that doesn't match this design doc here, but this all looks good regardless." type reminder would be ok. Obviously being diplomatic / soft skills is huge and the google doc addresses that a great deal.
But yeah especially if this is a non-trival change / lotta work ... not time to re-invent the wheel.
That's less-bad, but still a red flag :grimacing:
Clearly I never shipped stable code in the first two decades.
Code review is an essential part of shipping sound software.
I tell most of the juniors I work with to work defensively against this using a few strategies:
- smaller PRs. Break the work up any way you can. Make small tickets or submit your PRs with Part 1, part 2, etc. You can't release half-baked work, but you can usually find opportunities to split things up. Nobody should be submitting 20 file PRs and not expecting a lot of comments.
- Validate your strategy before building it. If there's a senior engineer in your team who nitpicks your code, get their buy-in before writing it.
- often bad code is the result of not really knowing how to approach a problem. Don't just write the code and slap together a PR. You might need to figure out a working solution, then go back and tweak it and refactor it before submitting it.
- review your own PRs and use a critical eye. You'll catch the low hanging fruit (like forgetting console.log calls). Every time you have the urge to leave a comment justifying a choice, question whether it's the right choice at all.
Personally, if I see a review that is >10 files that isn't explicitly a "Refactor" review, or I've been prepped ahead of time, it's probably going to take a long time to get the review out, because there are so many things to iterate on. I also have to block out a lot of time to even do the singular review, because it is so long and there is so much cognitive load to carry with it.
Smaller reviews are almost always better. If a review really can't be "completed" in a single PR, then I've also suggested 1/x reviews where bones are placed but gated under an FSS or something similar. This prevents code that shouldn't be run from being run until the whole feature (even if small) is complete, and lets the reviews focus on independent parts.
--
Essentially, I'm saying I disagree with you placing the "error" on the senior developers in this scenario. Burdening someone with insanely large reviews is the err of the submitter.
Firstly, if this is a brand new junior and they didn't have the guidance to avoid this problem in the first place, that's on the senior developer(s). These are on-the-job-learned skills, and the reason junior developers make less money is because they need guidance from seniors figuring out how all of that stuff works. Secondly, if they did have explicit guidance, have been advised to tighten things up a few times, yet can't swing it, the senior developer needs to be a senior developer and empathically help them work through whatever strategic block is causing the problem. Finally, if the junior has been told many times and not cleaned up their practices, the senior developer isn't doing them any favors by playing the role of rankled, imperious elder-- the person is likely not cut out for the role, or needs to spend more time learning, maybe as an intern. It needs to be addressed with their manager so the right person can fill that position.
If senior developers don't want to do that then they should work some place that doesn't hire juniors.
Completely agree. If I get a chance, I almost always try to have a conversation (Zoom, in person) instead of writing large walls of text. It's definitely discouraging to, really anybody, to see your review get slammed by someone.
Usually, if I see a common pattern or something is just wholly wrong, I try to whiteboard it out with them instead. I hope I've always come across nicer/a good mentor from this. I'm sure someone has disagreed :)
--
All in all I completely agree. Senior devs + managers need to set the stage, expectations, and provide the necessary mentorship/utilities needed to accomplish.
Also, a pet peeve of mine - one of my first teams I was on, a dev always commented on style issues. It got to the point where numerous junior devs complained and finally some other engineer stepped in and said "I don't disagree with your style comments, but you'd save yourself the headache if you just wrote a linter to catch that automatically." It's a pretty clear example in my mind of someone who finds self-importance in their voice being shown on each code review, when a simple utility would save everyone the headache.
Whenever there is a squabble over style in any PR in my team, I ask them to merge, decide afterwards on a single solution and then write a linter rule. Often a custom rule is necessary.
IMO the worst codebases to work on are those that have nitpickers that change taste all the time. Newbies join, try to "read the room" and find a lot of code that looks good, so they use it as a template. Only to be nitpicked because "we do things different now".
The seniors are not getting out of this scot-free when they barely make an effort to educate themselves, let alone others, or strategize ways to make this dummy-proof.
Almost everybody who started their career probably had their first code review torn to shreds. I'm not saying that's the right way to do it. But I will say, just like when a junior developer comes in thinking "well it works, so it's right", there are almost always other considerations than just the happy path.
A many-file, massive review might be 100% technically correct and flawless, and it's still going to take reviewers a lot longer to review than if they split it into 2-3.
--
> Merging side branch into side branch is a valid strategy
Completely agree! This is a fantastic strategy. You're not affecting the main branch, and generally people reviewing that specific branch can have the context of what your change is doing.
You also have to have a shared understanding of what's a reasonable request in a review, I've been in teams that relentlessly nitpick and teams that just "LGTM"s, eithers fine as long as you're agreed and consistent on what your team wants to do.
I think the opposite works better - submit thousands of lines of changes, you will get like 10 comments, address all of them, and then you're gold. The reviewer has no time to slog through thousands of lines of changes unless the code you're touching is their baby.
I think this hits the nail on the head. Code reviews have been adopted by most of the orgs I've worked at as a way to reduce any technical barrier to starting a project. Technical planning is moved from the start of a project to the end (or more accurately, to what has now become the middle). I'm certain there are code review processes that are done well, but they don't appear to be common.
It also brings significant additional overhead - generally every merge to master has to be tested individually, since there's no guarantee all PRs will be merged before release goes out.
I understand that code review has a place in enforcing code quality and training junior programmers. Here's what I would suggest. When you hire someone, make sure somebody is assigned to reviewing their code for the first few months. Once they've demonstrated their ability to write good code, from that point forward you trust them not to screw things up. In addition, foster a culture where people are open to criticism and proactive about fixing things. If someone sees a problem with someone else's code, they can either let them know informally, or just go in and fix it themselves. I think this approach would lead to much less red tape and much happier and more productive programmers.
It's not backwards. Code needs to be readable, well thought out, bug free. Code review (done well) helps establish these points.
> If you're writing a system that requires 5000 lines of code, it's a waste of your time to figure out how to break it up into 5 or 10 PRs
I guess if you think those 5000 lines are perfect that makes sense. Often big PRs in my experience have a lot of issues that the authors and other reviewers will have trouble catching.
> Once they've demonstrated their ability to write good code, from that point forward you trust them not to screw things up.
Ha! I don't even trust myself not to screw things up.
Anyway, if I’m reviewing and I see some improvements, I usually open a PR to the PR with my suggestions. I sometimes get halfway through it and realize why it is the way it is and never even leave a comment (and will defend the PR if someone else does a low-effort suggestion).
For instance, let's imagine this without the code review process and your subsequent advice on how to make it go better: People would just be merging in the 20 file PRs, sight unseen, without validating a strategy beforehand, without really knowing how to approach the problem, with the code just written and slapped together without tweaking and refactoring, and without any self-review using a critical eye to catch low-hanging fruit like leaving in console.logs calls. I think the "flawed" code review process seems like a much better outcome!
Code review culture is so important, and so often completely disfunctional. I've quit jobs because of toxic code review cultures like above, and one of the main things keeping me at my current job is sane code review culture we have.
* code review is expected responsibility, so everyone participates in every part of it regularly, so they are also incentivized to keep the process sane
* we have an auto linter and we recommend saving on fix specifically so no one argues about useless style nits
* CR back and forth is measured in minutes or hours so you are not waiting days to resolve someone’s drive by comment
* CR feedback always has a specific action item that is easy to address
* reviewees submit smaller CRs which are quick and easy to review for reviewers
Then CRs are pretty much pointless. The feedback I want as a senior developer is the complex stuff and that is half of the time not easy to address. The trivial stuff I usually, but not always, spot myself when checking the code before sending it for a review.
If you are doing system/algorithm design in the code review, it’s not meant for that.
The action item can also be “can we create a issue to track and discuss this further”
This is an essential component of a productive code review culture.
* This code should be changed looks bad - not a good comment
* This code should be chabged because ten nested ternaries gets hard to read - better
* This code is hard to read because there are ten nested ternaries. Can we replace it with a helper method that returns one value using if blocks? - best, in terms of actionability
Refactoring changes and such didn't have an approved Jira ticket, and you needed a ticket for every PR (even proposed PRs to show an idea). If you created a Jira ticket for the code improvement task, it would nearly always be set to lowest priority ("never" in practice) and the Jira ticket approval committee would rarely approve it for work in the next sprint unless it was accompanied by compelling business case backed by an enthusastic champion. A PM would never assign them, except as an onboarding practice task for someone new. You could self-assign them but you'd be taking a risk by working on something lowest priority.
In practice this meant people squeezed whitespace, simple refactorings and other improvements by comingling them into feature and bugfix changes. (Larger refactorings such as internal API and architectural changes, which that code sorely needed for ridiculously-bug-prone reasons and ridiculously-slow-development reasons, rarely got done at all.)
1. open a cr,
2. have your inexperienced friend approve it,
3. have the lead eng add 15 comments they caught,
4. merge the code anyway despite it breaking the core feature that’s being merged in parallel because you can’t just follow the design of the person who thought everything through and decided you knew better,
5. waste 8 more hours of your lead while they pair program with you, explain the consequences of your changes as you start to grasp that there’s a lot you don’t know, reverting many of the changes, implementing a cleaner approach, understanding why the lead commented what they did,
6. and hope they won’t get mad when they go back to fix merge conflicts on things that shouldn’t have been touched as they try to build the core..I don't have an adequate answer.
I don’t understand this line here, could you tell me why you decided to go this route?
Why are you using this function? Do you not know about the <design pattern/structure/API I would have used>?
Make sure to leave the author wondering what they could have done differently with their lives in order to avoid having to interact with you. But word it so harmlessly that they can’t tell if you’re genuinely interested in helping the team ship code or whether you’d push them under a bus if it helped you cross the street.
No better way to get a dev to question their sanity with a comment that makes them second guess their own goals.
There will be legitimate warnings and false-positives. You can gain some insights into how other people perceive and model the world, and in the process learn something, from both.
It's sad how a lot of people (including in the comments here) choose waste this opportunity by taking offense, and assuming that other people expressing their thoughts (which is an error-prone, lossy convertion process) are generally not well-intentioned.
Back when I worked for other people, I had the luxury (not a luxury) of being the mediator between the evil gremlin code reviewer and the wayward antihero programmers. Takeaways:
1. The evil gremlin code reviewer was almost always right, and was almost unequivocally the smartest programmer at the company.
2. Our antihero programmers not only tended to be wrong, they tended to be blatantly wrong in a way that worked on their machine, or worked for specific uses cases, but would never work in production.
3. The evil gremlin code reviewer was an asshole, liked being an asshole, and didn't care how many people thought he was an asshole.
4. The antihero programmers refused to learn how to be better programmers from the evil gremlin code reviewer, and the evil gremlin programmer refused to learn how to be educative, rather than castrative, in his code reviews.
5. Rather than serving as a quality enforcing function, most code reviews were spiked by the accounts team, meaning that evil gremlin code reviewer was in a perpetual state of frustration, antihero programmers were constantly in a cycle of post-production bug fixing/optimization, and the pace of development invariably ground to a halt over time.
6. No one cared how little I cared about this problem, but it was a nice excuse to not have to talk to the accounts team, and so I spent a lot of time listening to people whine.
There were no adults in the room.
Why do people expect to come on to a multi-100k-line project and just slap out a complex feature in a few days? Of course they'll end up with many dozens of comments from senior maintainers and everyone (including POs) will be very frustrated. It's pretty easy to avoid though by just following a sane ramp-up.
Early in my career, I had a code review with 2 Senior Engineers who absolutely hated each other. The code review session ended with them screaming at each other and them almost getting into a physical fight. Till today, I still have no idea about what they were fighting about but it pretty much boiled down to the naming convention of a local variable (I kid you not!)
Odd weeks: use this pattern
Even weeks: don't use that pattern I told you to use last time!
My own code: implement the pattern I said not to use in the last code review
Your unit tests are never good enough. Your integration tests won't match your production environment. Your canaries only test the happy path. If your goal is full CI/CD, some changes will make it through that will impact at least a subset of your customers in production without being caught.
A good code review process utilizes a larger portion of the team's understanding of a system, not just your own, to help catch some of these issues. This understanding could be system, product, or inter-team dependencies that you will never fully codify into an automated process.
It also socializes best practices, help folks learn new patterns and improve as software engineers.
However when it comes to quality assurance and correctness (if these terms collectively mean, preventing defects) then there's little evidence that it is an effective practice [0]. If the changes proposed are less than a couple hundred lines of difference and the reviewer is only reading one every couple of hours there's small but significant chance that they might catch an error. Humans are simply bad at this task.
[0] https://sail.cs.queensu.ca/data/pdfs/EMSE_AnEmpiricalStudyOf...
Why does the tech industry still rely on CR for this purpose? Probably because running empirical studies is time consuming and expensive. Instead we rely on the intuitions, experiences, and feelings of people, advice we get from others, etc.
A lot of stuff is codifiable as well. Shove your lint config in the repo, since you should all share a code style.