"This is completely false! Code is review is to ..." proceeds to state an opinion.
Sometimes, some days, I just look forward to not having to deal with programmer hubris ever again.
101–110 of 193 posts
"This is completely false! Code is review is to ..." proceeds to state an opinion.
Sometimes, some days, I just look forward to not having to deal with programmer hubris ever again.
No, the real reason for the code review is to protect the moat of senior engineers/leaders that would nitpick on minute details of code while ignoring the big picture to make sure they can gatekeep any promotions and their competition.
If that's how it functions where you work, I'd be looking for a new job.
Code reviews are still a critical step in most workflows. Though seems like everyone uses them for a different purpose: extra pair of eyes to meet a regulation/security, style police, and what this one says: maintainability.
And at the same time, code reviews are now a massive bottleneck in the development pipeline. Frankly, in a lot of teams they have been that for a long time. Though many would argue it was the only thing stopping absolute crap going into production.
But in a world where none of us writes code anymore, and I think we're there, even if the future is "just not evenly distributed" (Gibson), why should we have to do code reviews, the worst job of all?
Leaves me thinking that code reviews will land in the dustbin of history.
But for now I don't have a better solution.
What I find to be maybe the single most important part of code review is knowledge transfer. Our entire small team thumbs up a PR before it's merged unless there's a big rush on it, and this gives everyone on the team a rough idea of the state of the codebase at any given time. There's no being blindsided like "this whole system I depend on is gone" like I had happen at far more siloed places I've worked. Beyond that…
You describe a shared database, micro services you aren't aware of using the same database, and a desire for everyone to have a rough idea of the state of the codebase. And things breaking unexpectedly being caught by code review.
You have a bigger problem here of a system you can't reason about very well. The code review will help here, but I think you have bigger discoverability problems based on what I'm reading.
As others have said, what is needed is automated testing that would catch this sort of problem automatically without needing a human in the loop saying "Oh, wait...". We still want/need humans in the loop, but they should not be the only safety net.
That really depends on the quality of developers you are dealing with.
I certainly find myself assessing code quality, performance issues, shortcomings. Occasionally trivial bugs. Most importantly you review for taste, I'm tasting the code.
Maintainability is definitely important but is pretty subjective and is a subset of taste in general.
Earlier quoted context omitted.
User names match... are you the original author? Why commenting in the third person?
Because it was him... 10 months ago?
not "when the author wrote this, he was trying to say"
What I find to be maybe the single most important part of code review is knowledge transfer. Our entire small team thumbs up a PR before it's merged unless there's a big rush on it, and this gives everyone on the team a rough idea of the state of the codebase at any given time. There's no being blindsided like "this whole system I depend on is gone" like I had happen at far more siloed places I've worked. Beyond that…
> Our entire small team thumbs up a PR before it's merged unless there's a big rush on it This has been tried by a couple of my past managers. This feels great with a small team on a slow moving codebase. If you try to force it on a larger team or expect the codebase to move quickly then it turns into a performative game of skimming the code (if that) to click the thumbs up button so you can get back to your work. Th…
Then again, if you're dealing with an un-maintainable minefield of a code base then "fire the whole team, I dare you" might be what you're after.
This just makes reviewers and authors lazier. The purpose of code review is multi-faceted. Hard to maintain? Yes. Might have bugs? Yes. Can be done simpler/cleaner? Yes. Is in line with project code style? Yes. Get someone else to also understand the code? Yes. Onboard junior team member? Yes. Sanity check design decisions? Yes. This flippant note is mostly more self-justification for being a lazy code reviewer.
I've found the review and debugging process to be much more time consuming than writing/producing code, and just "praying it works" never ends well.
- a safety check to ensure that if a developer (or AI) goes rogue, it is more difficult to merge malicious code
- a second perspective from someone who isn't as close to the problem and might see a better way to do things, or problems that the original developer missed
- in some cases having someone more familiar with other parts of the system look at it who can tell if it won't interact well with something else
- ensuring there is at least one other person familiar with the code
- a learning opportunity. The author can learn from feedback from the review, and the reviewer can learn from the code in the change. Especially important when the author and reviwer have differing seniority. When I mentor a new employee, I add them as a reviewer to all my PRs so they can see how I do things, and review all their PRs so I can provide guidance. And sometimes I even learn things from them!
- yes, catching bugs, although this should not be the primary mechanism for that, and I agree is not the most important reason. It is especially important for security and performance bugs though, as those are harder to catch with automated testing.
What I find to be maybe the single most important part of code review is knowledge transfer. Our entire small team thumbs up a PR before it's merged unless there's a big rush on it, and this gives everyone on the team a rough idea of the state of the codebase at any given time. There's no being blindsided like "this whole system I depend on is gone" like I had happen at far more siloed places I've worked. Beyond that…
> For instance recently I made a small change to a table and a coworker pointed out that there was a microservice I wasn't considering that wrote to that table that would break If code reviews are important, where does testing sit? Presumably if the coworker had not been part of the code review something would have stopped the breaking change making its way to prod?
At least, that's what people do by default.