6 ms·
How to run a miserable code review
- wkimeria 3y agoLaughs in recognition 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!)
- archibaldcecil 3y ago[dead]
- EvelynJohnson 3y ago[dead]
- notjustanymike 3y agoSo 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!
- rabbits_2002 3y agoDo you work at the same place I do?
- adhesive_wombat 3y agoBonus 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...
- asu_thomas 3y ago> 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.
- bornfreddy 3y agoTo 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.
- bluefirebrand 3y agoI think "fyi" comments are valuable in PRs. 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
- desi_ninja 3y agoAlso nit:
- bluefirebrand 3y ago
- fuzzy2 3y agoWhat, 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.)
- kxrm 3y agoThe best is when the only possible reviewer is your Engineering Manager because you have no other engineers in your domain on the team.
- Maxion 3y agoThe 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.
- hobs 3y agoThat's lucky to have a manager who even logs into GitHub. Many jobs of mine I have been my own approver.
- kxrm 3y agoI 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.
- pak9rabid 3y agoAnd in the case of merge conflicts, steamroll your changes over everyone else's, as you know what's best and everybody else is wrong.
- kiitos 3y agoMerge 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.
- agentultra 3y agoMinimum 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.
- cjfd 3y ago5 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.
- withinboredom 3y agoI have an open PR from my first week. Not a single review. I’ve merged entire features since then…
- polishdude20 3y agoDo you specifically ping people to review? At my company we've got a habit of not only assigning reviewers in GitHub but also pinging them in slack.
- withinboredom 3y agoIt’s a dev environment improvement so I pinged the whole team. Nothing like a chilly start at a new company eh?
- duxup 3y agoCode 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/ 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.
- s__s 3y agoIf 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.
- duxup 3y agoI 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.
- polishdude20 3y agoI 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.
- JKCalhoun 3y agoUsually no one is looking for design feedback, but plenty of offers....
- duxup 3y agoYeah. 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.
- JKCalhoun 3y agoI never knew a code review until maybe the last 5 or 6 years of my career. Clearly I never shipped stable code in the first two decades.
- kiitos 3y agoStable? Maybe. Maintainable in a business-continuity sense? Possible but unlikely. Code review is an essential part of shipping sound software.
- deleted 3y ago[deleted]
- stcroixx 3y agoMore time wasting busywork of dubious value.
- JKCalhoun 3y agoMost 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.
- throw1234651234 3y agoThere is a good article approaching it from the other side here: https://mtlynch.io/code-review-love/ https://mtlynch.io/code-review-love/ - I am not the author, but it's one of the few things I have bookmarked.
- digging 3y agoI 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.
- jlokier 3y agoThat 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.)
- throw1234651234 3y agoWe 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.
- marktangotango 3y agoI 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.
- fwungy 3y agoIt's a good way to game performance reviews.
- ashtonbaker 3y agoWhat makes you call your current review culture “sane”? I’m not sure I have seen that yet in my career.
- bobthepanda 3y agoYMMV but i work at a place where * 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
- wkimeria 3y agoThis right here! It makes for a sane, friendly culture, and junior team members also get to learn during the code review process.
- beeskneecaps 3y agoTriggered. 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.
- datavirtue 3y agoOr they open up the floodgates after that, listing numerous other unrelated "issues" with your code. In my mind you get one stab. After that STFU.
- deleted 3y ago[deleted]
- fwungy 3y agoFlag 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.
- lifefeed 3y agoRemember 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.
- agentultra 3y agoAlso, 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.
- tonymillion 3y agoThe mark of a 10x dev is the ability to script an auto-gaslight-comment of “Are you sure that’s what you intended?”. No better way to get a dev to question their sanity with a comment that makes them second guess their own goals.
- schwartzworld 3y agoCode 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.
- danparsonson 3y agoAlternative 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!
- SilverBirch 3y agoTotally 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.
- 3y ago
- PartiallyTyped 3y agoHow to make your colleagues miserable 101. 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..
- Capricorn2481 3y agoHow does a company even allow a person that does this to work there?
- deleted 3y ago[deleted]
- deleted 3y ago[deleted]
- PartiallyTyped 3y agoI have spent a lot of time thinking how to respond to this. I don't have an adequate answer.
- braza 3y ago(honest question, please do not downvote): Why the tech industry still relies in the CR as a quality assurance/correctness process?
- bobthepanda 3y agoReview 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.
- braza 3y agoBut in this case, I think it might be related with the background/knowledge/ways of coding difference between the maintainers?
- bobthepanda 3y agoIf you’ve worked together long enough, hopefully you are all coding in a common way that everyone can read. A lot of stuff is codifiable as well. Shove your lint config in the repo, since you should all share a code style.
- dj_gitmo 3y agoBy 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.
- jeltz 3y agoBecause 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).
- robofanatic 3y agoIf 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.
- thegrimmest 3y agoI 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.
- debacle 3y agoA lot of this is hyperbolic whining. 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.
- bretthardin 3y agoEverything is a people problem - even code review. SWE who choose to not grow (by refusing to learn) are going to create awful culture.
- candiddevmike 3y agoFire the gremlin, it's never worth picking an IC over the team morale. Folks can learn to be better programmers, assholes will always be assholes.
- atq2119 3y agoThe problem with this advice is that, at least in this story, if we're taking it at face value, the antihero programmers didn't learn.
- alxmng 3y agoI’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.
- mbwgh 3y agoOver 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.
- napierzaza 3y ago[dead]
- JohnMakin 3y agoMy 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.
- switch007 3y agoMy favourite is: 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
- jollyllama 3y agoNumber 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.
- ajhurliman 3y agoDon’t forget blocking merges based on your personal opinions about the UX design which haven’t been communicated to or approved by the design team.
- butrosbutros 3y agoMakes me miss Klaviyo :)
- thenoblesunfish 3y agoThis 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.
- nancymark962 3y ago[flagged]