(honest question, please do not downvote): Why the tech industry still relies in the CR as a quality assurance/correctness process?
How to run a miserable code review
31–40 of 122 posts
Re: How to run a miserable code review
#32Code 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 wi…
> 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.
Re: How to run a miserable code review
#33I 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 eith…
What makes you call your current review culture “sane”? I’m not sure I have seen that yet in my career.
YMMV 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
Re: How to run a miserable code review
#34(honest question, please do not downvote): Why the tech industry still relies in the CR as a quality assurance/correctness process?
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.
Re: How to run a miserable code review
#35Earlier quoted context omitted.
> 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.
I 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
Re: How to run a miserable code review
#36Earlier quoted context omitted.
What makes you call your current review culture “sane”? I’m not sure I have seen that yet in my career.
YMMV 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…
> * 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.
Re: How to run a miserable code review
#37Code 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 wi…
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.
Re: How to run a miserable code review
#38(honest question, please do not downvote): Why the tech industry still relies in the CR as a quality assurance/correctness process?
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.
But in this case, I think it might be related with the background/knowledge/ways of coding difference between the maintainers?
Re: How to run a miserable code review
#39If 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.
Re: How to run a miserable code review
#40So 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…
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.