So many missed opportunities for a small business.
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!
Bonus if the vague comments relate to some refactoring that you'd like to do, the details of which exist only in your head. Meanwhile, yes, you know the existing code is like X, and has been like that for years, but obviously I shouldn't have followed it and it's all wrong. For details on how I'd like it to be, please read your mind.
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...
> Bonus if the vague comments relate to some refactoring that you'd like to do
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.
To be fair, it's not always a bad idea to present such ideas. However you need to make the comment resolvable, or better yet, create an issue for refactoring, add a small "fyi" comment and let them resolve it when they read it. This way they are aware of possible upcoming changes to this code but the MR/PR is not blocked.
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
Also such "fyi" comments can be a great way to get the refactoring discussion outside of just your own head. It can be a good start to boiling down the reasons for your refactor and getting external buy-in/validation that you are thinking down a useful path to the entire team and not just your own ego.
I often add multiple comments and then approve the pr. My main question is always: does this make the codebase overall better or worse for some definition of good? If that bar is met, iterative improvements should not block deployment/merge.
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.
I always make sure that author can resolve my comments. If they can't do that, the pr is not approved - because I would like them to address something. If I approve it, then each of my comments should be resolvable.
What, those are rookie numbers! I opened a pull request 13 business days ago. I even rebased it multiple times when conflicting code was otherwise merged. There’s been no feedback whatsoever. Everyone even remotely involved is now on vacation.
(It’s not like I care either, the project sucks anyway.)
The best is when you are the technical lead with admin rights, but there's no one on the team senior enough to actually want to review your code. You leave itfor two weeks when the person who should be PO, but is never in any meetings, doesn't look at the tickets or the pull requests, and only reachable by email calls you asking where the feature is.
I think it is a huge assumption to believe my Engineering Manager even has time to review anything within the next 2 weeks. Every time I have hit this at my company I have had a minimum wait of almost 3 weeks on average.
However, yea, against just approving your own code, this is better.
Merge conflicts to the main branch? I've never even heard of an org where it's OK to force-push to main. That's wild! And a huge red flag, that ought to be fixed at priority zero. Wow.
Minimum 5 days hurt. Too real. If there’s one way to destroy productivity and morale it’s to leave a vague comment on a review and not come back to it for a few days.
5 days is very quick. I have an open pull request that has been open for 6 months or thereabouts.... :-( No, I am not suggesting this as a good practice.... Actually, I would say that a pull request should be reviewed in 24 hours. Pull requests made on Friday may wait until Monday.
Code reviews seem like a hellscape of "could do X".
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.
If the code works let it go? That’s an incredibly low bar to set. Unless you’re working on an all-star team, that approach will quickly get your codebase in a really bad place with incredible amounts of technical debt.
I think you have to read their process to be clear. They don't want to hold up things over style quibbles and etc. It is something they're ok with talking about, but not every aspect that could be improved is a hard stop.
I feel like if you're looking for design feedback during the code review that's already way too late. There should be much more communication going on before the coding even happens.
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.
For anybody else curious (maybe this was just new to me), the abbreviation CL (used all over the place in the linked docs but never actually defined anywhere I could find) stands for change log, or change list, near as I can tell. Am I the only one out of the loop here?
Most of the changes I have seen in software engineering over my career have not been for the engineers but have been in service of a nervous management that are desperate for numbers and reins.
There is a good article approaching it from the other side here: https://mtlynch.io/code-review-love/ - I am not the author, but it's one of the few things I have bookmarked.
I find this much more valuable than the main post, which is entertaining but leans too hard into absurdity to be a useful guide for what actual problematic behavior looks like.
That was a fun read. I found item 6 interesting, "Separate functional and non-functional changes". Because that's obviously the sensible and useful thing to do. But at the last job, whitespace, reformatting and simple refactoring changes were strongly discouraged unless they were part of another functional change. There was a written policy about it.
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.)
We have had similar issues. At the end of the day, it's just a bad PO issue imo. And yea, in those situations, it very much becomes a "refactor-as-you-go" type of deal, but we do try to keep those changes very small and clear so it doesn't interfere with the main logic. It actually gives me a bad taste to even type it, because not isolating formatting/clean up really does lead to bugs in the long run.
I once worked at a place where about 5 of 15 developers sat on crucible all day fellating each other on code reviews. Anytime the rest of the developers would have code reviewed they'd be met with a long list of required changes that where "standards" the 5 had agreed on, and never communicated out side of their own comments in the tool. And the "standards" changed often, and the changes where never communicated either. Besides being a toxic culture, it was a real drag on development velocity to say the least.
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
> * CR feedback always has a specific action item that is easy to address
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.
Reviewers are responsible for not just pointing out issues, but also providing (at a minimum) some form of direction, or (more ideally) one or more explicit suggestions as to how to resolve those issues.
This is an essential component of a productive code review culture.
Yeah, a good review must explain why, and should ideally explain how it should be instead if it needs explaining.
* 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
Prioritize code reviews over development work. Have SLAs. If you are assigned a CR, get to a good stopping place, pause your work, do the CR, and resume. Close CRs that have become stale due to submitter abandonment.
Triggered. Missing from this is the experience where the reviewer rejects for something trivial, then you fix the issue in 1 minute, resubmit, and finally have to wait 24 hours for them to check their code review requests again.
Flag the code and unit tests for multiple petty issues and then forget to review your change requests. Eventually accept them weeks later without comment after multiple prompts from the developer.
Remember that your goal is to have something to say, not to make the code better. No matter how good the code is, you need your fingerprints on it somehow.
Also, if you have a bad manager who likes to gaslight you with questions that make you doubt yourself, a code review is a great way to pass on the good feelings to your co-workers. Make sure to ask plenty of leading questions that interrogate the competence of the author. The more passive-aggressive the better. And word it so that you can deny anyone that calls you out for negative behavior.
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.
Code review is a flawed process, especially for junior engineers. You spend time and effort, maybe days of work, getting this to work, and then some know-it-all leaves a bunch of nit-picky comments or tells you you're doing it wrong.
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.
Alternative view - if you approach a code review (or indeed any similar process) as a competition to be won or some kind of personal validation process, then you're likely to be disappointed and are potentially wasting an opportunity to learn something. Especially as a junior!
Totally agree it's flawed, but very often the reason it's flawed is because something has gone wrong somewhere else. Yes, it's very common for someone to do a PR and for someone else to turn around and go "Oh god, why the hell did you do it like this?!" - Fundamentally challenging the entire purpose of the code. What's happening there is the code review is just highlighting the fact you didn't have a design review.
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.
Honestly, you sound like an asshole here. Do you realize that Adderall is a medication? Do you realize how fucked up it is to call someone mentally defective as if your brain meet some standard of perfect functioning? (it doesn't.)
I dunno mate, I think adults should be able to handle being chortled at. You're responsible for your own emotional state. If you've done something laughable (or even if you haven't), then don't be surprised when people laugh. Taking it personally is optional and not recommended.
I agree that people shouldn't be too thin-skinned, it's still important to stay civil and supportive.
Being able to clearly and unambiguously convey that a change sucks while not hurting the other person's feeling is a useful skill to practice. (It's okay to fail at it, too, as long as you're aware that that's almost always a weakness.)
The issue is the broad, ambiguous, definitions of things like "civil". In many cultures (eg. French, Russian) a direct, no-nonsense communication style is the default. You can just say things like "Your idea is hilariously stupid" and no one really gets bent about it. In the world of multicultural, international business, this seems like the most viable default, simply because it's the easiest to codify and communicate. Trying to teach very culturally-specific minutia of civility to a diverse staff just gets in the way.
I'd rather work somewhere the burden of not being offended is placed on the listener, not the speaker. Somewhere we can just say what we think.
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.
You've probably heard this before, but it's worth repeating: code review doesn't have to be a "flawed process" that is "nit-picky" -- if it is, there's a problem in your organization that needs to be -- and absolutely can be -- fixed!
> 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.
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.
Everything you listed are exactly what the "know-it-all" senior developer is expecting already, though. If you're doing these things before you submit your review for eyes to look at, you'll come to a point where your reviews should only go through 0-2 revisions before submission is ready.
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.
Counterpoint, most places do not teach individuals how to check in often and make their stuff smaller. Some of them don't even realize merging side branche into side branch is a valid strategy to avoid merging incomplete features into the main branch(es).
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.
I agree the onus is to establish a clear culture, guidelines, and mentorship to newbies and junior developers in this regard.
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.
I agree that the coding expectations are fine-- and I'm not saying you do this-- but conveying annoyance through pedantic, overly nitpicky, or snarky junior code reviews is a management and mentorship failure. In any field, someone consistently acting like a know-it-all is indulging their own emotional shortcomings under the guise of enforcing good practices.
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.
> but conveying annoyance through pedantic, overly nitpicky, or snarky junior code reviews is a management and mentorship failure
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.
Yes, I agree with all of that. Additionally, it's important that management understands that seniors need the time to actually do these things. You don't just get whatever percent less efficiency with junior developers, it takes time from your senior developers doing their work to help the juniors along.
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".
If the code review process is (through your supportive wisdom) pushing the people you work with to follow those practices, then I would argue it is actually a pretty good process. Those are all very good things to do, that a good code review culture should definitely be pushing people toward.
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!
I always tell juniors to remember that it is their code until it gets merged in. Feel free to tell people “no” and why (“addressing” a comment doesn’t necessarily mean the code has to change). If the reviewer doesn’t like it, they can do it themselves after it gets merged in or create a PR to the PR. There are only a few cases where they can actually get blocked: leaking PII, security issues, and egregiously bad code (like writing something 5x times instead of using a loop).
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).
This is backwards and why I think mandatory code review is a waste of everyone's time. Nobody should be writing code with the goal of passing a code review. They should be writing code that solves a problem and adds value to the end product. 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 (which might actually make it harder to review, since chunking it up would remove context).
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.
> Nobody should be writing code with the goal of passing a code review.
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.
I disagree about smaller PRs being always better. I often find them more difficult to review if they contain only small part of the feature - I see something and ask - why is this like this? Oh, it's used like that in a subsequent PR... which then forces me to look up that second PR and review them together anyway.
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.
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..
Review can also effect maintainability; the code should ideally be parseable by other people on the team with little context of the specific project. And it should ideally spread knowledge of the code to at least one other person.
By CR do you mean meetings where everyone talks about the code? Or a github-style pull-request?
I personally find meetings where we talk about a pull-request to be a poor use of time. I want to have time to read, reread, and interact with the code.
Because when done in a team where people want to work together and do not have incentives discouraging that it is a really good tool for spreading knowledge and improving maintainability. So far I have only once worked at a place where the review culture was bad and that was due to issues much bigger than the code reviews (specifically a culture where a different team wrote code for another team to maintain, encouraging shoveling shit code over the fence).
There are a lot of qualitative reasons: it helps spread knowledge of changes in the code, it helps maintain a consistent style, shares knowledge, etc.
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.
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.
Test automation is also written by humans, so while there are some improvements to be made for productivity, it’s definitely not foolproof, so it’s good to have multiple layers of defense.
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.
If you are the manager of this team, make sure you ask the developer why there are so many comments on the PR without trying to understand what those are and question the quality of their work. Keep repeating same thing on every PR no matter how many times the developer explains.
I agree that this post outlines a miserable code review, but where I work I often see the opposite: a miserable submission. Is it wrong to expect engineers new to the project spending a few whole days (read 8-24 full hours) just reading the documentation, code, tests, project layout, etc.? This time would be invaluable to avoid reimplementing things that already exist, following existing patterns and styles, understanding where to put things, etc.
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.
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.
I’ve experienced this more than once. I think code review is best used only for finding defects (bugs). Otherwise, it consists of demoralizing comments and bikeshedding.
Over time, a programmer gathers a personal set of quality heuristics that has worked for them. A code review is an opportunity to sanity-check your changes by running your code against another person's set of heuristics.
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.
My favorite is when you submit a PR full of conventions that are all over the code base that you yourself didn't establish, agree on, or write yourself, and the reviewer uses your PR as a platform to lambast the conventions that again, were not yours.
Number one thing is that the scope of code review needs to be defined for the team. IMO it's best to scope to a few, key stylistic constraints and have senior engineers watch for red flags. Managers should actively tie-break on matters of "premature optimization" because they're actually responsible for allocating the team's time. "Does it work? Yes." should be a veto against any other blockers. Anything more is inviting office politics into the process.
This is very funny but thank god not my experience at all. Code review should be, and is where I work, about making constructive comments and checking that the code does what its supposed to do. If at code review, people are really questioning the purpose of things and being obstructionist, an earlier stage of planning and design has already failed and code review itself is not to blame.
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!)
121 comments
[ 3.4 ms ] story [ 190 ms ] threadA 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.
That's less-bad, but still a red flag :grimacing:
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.
Clearly I never shipped stable code in the first two decades.
Code review is an essential part of shipping sound software.
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.)
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.
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
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”
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.
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.
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.
Being able to clearly and unambiguously convey that a change sucks while not hurting the other person's feeling is a useful skill to practice. (It's okay to fail at it, too, as long as you're aware that that's almost always a weakness.)
I'd rather work somewhere the burden of not being offended is placed on the listener, not the speaker. Somewhere we can just say what we think.
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.
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.
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.
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".
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!
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).
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.
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 don't have an adequate answer.
A lot of stuff is codifiable as well. Shove your lint config in the repo, since you should all share a code style.
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.
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.
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.
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.
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.
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
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!)