101–110 of 122 posts
Earlier quoted context omitted.
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.
That's less-bad, but still a red flag :grimacing:
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.
Code review is an essential part of shipping sound software.
Earlier quoted context omitted.
What makes you call your current review culture “sane”? I’m not sure I have seen that yet in my career.
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.
(honest question, please do not downvote): Why the tech industry still relies in the CR as a quality assurance/correctness process?
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 l…
No better way to get a dev to question their sanity with a comment that makes them second guess their own goals.
Earlier quoted context omitted.
The best is when the only possible reviewer is your Engineering Manager because you have no other engineers in your domain on the team.
That's lucky to have a manager who even logs into GitHub. Many jobs of mine I have been my own approver.
However, yea, against just approving your own code, this is better.
Earlier quoted context omitted.
> * 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.
* 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
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 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…
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.
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 wi…
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: Peopl…