logoalt Hacker News

jascha_engyesterday at 9:16 AM2 repliesview on HN

> well-aligned developers

I think this is very key, if the development style and the direction of the project is clear, much less review and alignment is necessary.

And also

> avoid keeping branches open for more than a day

Big +1 on that, fast reviews are extremely key. Most teams I have seen often took days or even weeks to merge branches though, often because you end up waiting too long for reviews in the first place. Or because of good old bike-shedding. But also because these code reviews often uncovered uncertainties that needed longer discussions.

However usually code is easy to change, so defaulting to "just merge it" and creating followup tasks is often the cheaper approach than infinite review cycles.


Replies

dxdmyesterday at 9:35 AM

I think it's still worth-while to do reviews. A second pair of eyes does wonders, and it spreads knowledge of what things exist and how they work. If changes are small, reviews can be quick. It's possible to keep building on top of code being reviewed, and even easy when using modern VCS tooling like jujutsu.

Once the code is merged, chances are it will not get changed Those follow-up tasks will be displaced by more pressing work that will keep piling onto a slightly unstable foundation, increasing the tilt over time.

There is an excluded middle between "no reviews" and "infinite review cycles": proper, timely and efficient reviews. They are worth investing the time to get right. They will start paying dividends months down the line, and boy will they keep paying.

This is not about trying to get things perfect from the get go, but to get them done right while you're there. "We'll fix it later" is not gonna happen, and is much more expensive than it initially seems.

KronisLVyesterday at 9:52 AM

> However usually code is easy to change, so defaulting to "just merge it" and creating followup tasks is often the cheaper approach than infinite review cycles.

I wish this was the "default" mindset everywhere, especially in those cases where you have that one colleague that loves to nitpick everything and doesn't see an issue with holding up both releases and wasting your time over menial pedantic stuff. It would be so much easier to merge working code and let it work, and keep those TODOs in the backlog (e.g. trash).

In a sane world, code review would be like:

  1. Will this work and not break anything? We checked it, it's okay. There are no apparent critical or serious issues here.
  2. Here's a list of stuff that you can change if you wish, here's why it might be an improvement.
  3. Okay, we have some left-over nice to haves, let's keep track of those for later (or not) and merge.
It gets infinitely worse if you're working on 3 projects in parallel and the person wants long winded calls or starts nitpicking about naming, or wants things done exactly their way as if it's the only way (doubly worse if their way is actually worse by most metrics and tastes).