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…
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.
How to run a miserable code review
111–120 of 122 posts
Re: How to run a miserable code review
#112Code 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…
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 r…
Re: How to run a miserable code review
#113So 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.
Re: How to run a miserable code review
#114Earlier quoted context omitted.
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.
Stable? Maybe. Maintainable in a business-continuity sense? Possible but unlikely. Code review is an essential part of shipping sound software.
Re: How to run a miserable code review
#115There 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.
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.)
Re: How to run a miserable code review
#116Re: How to run a miserable code review
#117Early 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!)
Re: How to run a miserable code review
#118Earlier 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…
Re: How to run a miserable code review
#119Re: How to run a miserable code review
#120There 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.
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, a…