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.
How to run a miserable code review
71–80 of 122 posts
Re: How to run a miserable code review
#72Earlier quoted context omitted.
Fire 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.
The 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.
I read it as weren't given the opportunity to learn, and didn't a have a mentor to teach them.
Re: How to run a miserable code review
#73Number 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.
Re: How to run a miserable code review
#74My 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
Re: How to run a miserable code review
#75Earlier quoted context omitted.
And in the case of merge conflicts, steamroll your changes over everyone else's, as you know what's best and everybody else is wrong.
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.
Ah, but it wouldn’t be something as inelegant as a force push. No no. You do merge. It’s just that the outcome is slightly… biased.
Re: How to run a miserable code review
#76How 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…
How does a company even allow a person that does this to work there?
[deleted]
Re: How to run a miserable code review
#77So 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 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.
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.
Re: How to run a miserable code review
#78Earlier quoted context omitted.
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.
I have an open PR from my first week. Not a single review. I’ve merged entire features since then…
Do 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.
Re: How to run a miserable code review
#79So 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…
I 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.
Re: How to run a miserable code review
#80Earlier quoted context omitted.
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.
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.
Usually no one is looking for design feedback, but plenty of offers....