3 ms·

This kind of greedy optimization approach is fine if all you’re just trying to maximize velocity, but it starts showing cracks on a long enough timeline in suff

This kind of greedy optimization approach is fine if all you’re just trying to maximize velocity, but it starts showing cracks on a long enough timeline in sufficiently large or complex projects. Speed without alignment on direction leads to ugly, tangled messes. As with most quality issues, the key is to try and surface quality problems as early as possible. Finding a bug in prod is worse than finding it in PR is worse than finding it with tests is worse than finding it at your desk is worse than finding it in the design. Engineering teams should choose an operating point in their processes that balances quality and velocity to match the business needs of their project. Most often I’ve seen slow PRs caused by stuffing too much of the responsibility for software quality into that single step. In my team we prefix nitpicky comments explicitly with “nit:” and it’s up to the author to decide what to do with it. We minimize trivial nits by enforcing a style checker. We also discuss in retro if the PRs are too big to review effectively or if PRs are turning up comments on solution architecture, because at that point it’s too late—the author likely should’ve separately discussed the architecture before it got to a PR.

wubrr 3y ago
Generally agree, but at some companies/teams the ratio of 'nitpicky subjective feedback' to 'you have a bug here feedback' is like 95:5 while at others it's 5:95. The latter function much better.
poolopolopolo 3y ago
MRs are bad place to align things at scale.
- creshal 3y ago
  They should be a last resort for that sort of alignment, but it's still important to pull that last resort if it's necessary, so you don't end up like Cloudflare and let your own standards slip so badly that you end up with a 100% preventable multi-day outage.
zdragnar 3y ago
> In my team we prefix nitpicky comments explicitly with “nit:” and it’s up to the author to decide what to do with it This is different from what the OP is talking about. I've worked at places where staff engineers seem to have been rated on number of comments left on PRs... they were typically somewhere between nit picky and useless, with the occasional person directly contradicting feedback they'd given in a previous PR. Oh, and you had to address every one of them before getting approval.
- Reubend 3y ago
  > I've worked at places where staff engineers seem to have been rated on number of comments left on PRs > Oh, and you had to address every one of them before getting approval. This just sounds horrible.
- d0mine 3y ago
  > rated on the number of comments It sounds idiotic. It is unrelated to the question whether short-term (one PR) only reviews are preferable over reviews that recognize the reality that it is not the last PR ever and therefore more long-term view may be useful.
hot_gril 3y ago
Long-term project health rests on bigger things like API design, database choices, or service layout.
- uoaei 3y ago
  Hopefully these are in-scope when it comes to accepting or rejecting PRs and are not just considered incidental to whatever the stated purpose of the PR is. What you listed appears to be more fundamental system design concerns so hopefully they are candidates for further review.
  - hot_gril 3y ago
    It's usually out of scope by the time you're writing a CL, which is fine because there's design review. The issue is about priorities. Nitpicky code review can be very expensive, and I'm not convinced it brings noticeable benefit. I've seen team-quarters wasted due to bad large-scale decisions. Sometimes the truth doesn't come out until the the real thing is in production, and all these extra burdens make it harder to change course. I think people are starting to get this because the amount of code nitpicking or caring about small things in general has gone way down.
throwawaaarrgh 3y ago
> it starts showing cracks on a long enough timeline in sufficiently large or complex projects Show me a project which this doesn't happen to on a long timeline and I'll show you a project with no users and perfect programmers. Entropy is unavoidable. It will eventually degrade despite your best efforts and in the meantime you aren't benefiting from a working feature. We should try to delay software entropy as much as practical, but not at the expense of shipping. Basically there is a balance to strike. I don't have a formula for it, but I do know that merging often ends up being more valuable than merging later just to ensure clean code. Working features are more valuable than a mess. A mess sucks, so we should try to avoid it, but the world runs on messes, not clean code.
- danielovichdk 3y ago
  Read the Redis code base please.

wubrr 3y ago

Generally agree, but at some companies/teams the ratio of 'nitpicky subjective feedback' to 'you have a bug here feedback' is like 95:5 while at others it's 5:95. The latter function much better.

poolopolopolo 3y ago

MRs are bad place to align things at scale.

creshal 3y ago
They should be a last resort for that sort of alignment, but it's still important to pull that last resort if it's necessary, so you don't end up like Cloudflare and let your own standards slip so badly that you end up with a 100% preventable multi-day outage.

zdragnar 3y ago

> In my team we prefix nitpicky comments explicitly with “nit:” and it’s up to the author to decide what to do with it This is different from what the OP is talking about. I've worked at places where staff engineers seem to have been rated on number of comments left on PRs... they were typically somewhere between nit picky and useless, with the occasional person directly contradicting feedback they'd given in a previous PR. Oh, and you had to address every one of them before getting approval.

Reubend 3y ago
> I've worked at places where staff engineers seem to have been rated on number of comments left on PRs > Oh, and you had to address every one of them before getting approval. This just sounds horrible.
d0mine 3y ago
> rated on the number of comments It sounds idiotic. It is unrelated to the question whether short-term (one PR) only reviews are preferable over reviews that recognize the reality that it is not the last PR ever and therefore more long-term view may be useful.

hot_gril 3y ago

Long-term project health rests on bigger things like API design, database choices, or service layout.

uoaei 3y ago
Hopefully these are in-scope when it comes to accepting or rejecting PRs and are not just considered incidental to whatever the stated purpose of the PR is. What you listed appears to be more fundamental system design concerns so hopefully they are candidates for further review.
- hot_gril 3y ago
  It's usually out of scope by the time you're writing a CL, which is fine because there's design review. The issue is about priorities. Nitpicky code review can be very expensive, and I'm not convinced it brings noticeable benefit. I've seen team-quarters wasted due to bad large-scale decisions. Sometimes the truth doesn't come out until the the real thing is in production, and all these extra burdens make it harder to change course. I think people are starting to get this because the amount of code nitpicking or caring about small things in general has gone way down.

throwawaaarrgh 3y ago

> it starts showing cracks on a long enough timeline in sufficiently large or complex projects Show me a project which this doesn't happen to on a long timeline and I'll show you a project with no users and perfect programmers. Entropy is unavoidable. It will eventually degrade despite your best efforts and in the meantime you aren't benefiting from a working feature. We should try to delay software entropy as much as practical, but not at the expense of shipping. Basically there is a balance to strike. I don't have a formula for it, but I do know that merging often ends up being more valuable than merging later just to ensure clean code. Working features are more valuable than a mess. A mess sucks, so we should try to avoid it, but the world runs on messes, not clean code.

danielovichdk 3y ago
Read the Redis code base please.