Hacker News Reader

Comment by epage | Hacker News Reader

Agree with the overall sentiment but disagree with

> A good rule of thumb is 300 lines of code changes - once you get above 500 lines, you're entering unreviewable territory.

I've found LoC doesn't matter when you split up commits like they suggest. What does matter is how controversial a change is. A PR should ideally have one part at most that generates a lot of discussion. The PR that does this should ideally also have the minimal number of commits (just what dossn't make sense standalone). Granted this take experience generally and experience with your reviewers which is where metrics like LoC counts can come in handy.

View on HN

It’s often an inverse correlation in well functioning environments. Controversial changes and bug fixes are small and targeted and are deeply reviewed for unintended side effects, while large change sets are often new work that’s been discussed upfront and follows well established patterns and gets waved through.

Good luck getting 90% of devs to commit (har har) to this level of history surgery. Of the ones that actually know how to do it (a small fraction of your typical engineer) an even smaller fraction of that is going to have the patience to do it correctly. You’ll tell devs to do this kind of thing and you’ll either have their eyes glaze over from lack of understanding, annoyance at the extra work, or nodding then apathetical disregard. The one top engineer will do it then be frustrated that the rest of the org doesn’t do proper commit hygiene.

It’s not really something you can easily enforce with automation, so basically unachievable unless you are like Netflix and only hiring top performers. And you aren’t like Netflix.

You also need tools that support the workflow. I love small self-explanatory commits. In git, it's easy to do. Recently I switched to a Perforce organization and it's a disaster. P4 doesn't support stacked CLs and it dramatically hurts engineering quality. Everyone lands mega-CLs because it's the only supported workflow.

Have you tried using the git adapter to Perforce backend [1]? It should let you avoid mega-CLs. When Google used to use Perforce, an internal somewhat similar but a bit more advanced tool git5 was very popular.

[1] https://git-scm.com/docs/git-p4

The adapter breaks down on sufficiently massive repos. Unfortunately, there's a reason Perforce is still in use despite no one liking it. Git scales via sub-repos, but those have their own drawbacks, and no one is enthusiastic enough about this to champion it.

With the right workspace mapping, you can pull in just part of the depot, which makes the repo size a lot more manageable.

Sorry to hear about the use of Perforce. Even the dinosaur of a company I used to work at shifted away.

(granted, I know VCS like it are still good for assets)

In some parts of the industry number of CRs and revisions-per-cr is tracked as a performance metric.

Many people learn to game this to make their "numbers" appear good i.e. high number of CRs and low revisions per CR.

Easy and fun to put metrics around random things. I'll start to squirm if you ask me to draw the connection between these metrics and NPV.

The interesting phenomenon is the discovery of gamification of such metrics.

I do see the value in breaking down larger chunks of work into logically smaller units of work and then produce multiple pull requests where needed.

But some people are really clever and influential and manage to game these numbers into "apparent success".

It just becomes a huge cycle which is easier for everyone involved than doing actual work that benefits your customers.

Reply on news.ycombinator.com