Hacker News Reader

Comment by kiitos | Hacker News Reader

The thing that's missing from this (and most) articles about the described kinds of code optimizations is context: when is it appropriate to make these kinds of optimizing changes, and when is it not?

> [with] this change ... the speed improved significantly [but] the readability and ease of coding decreased quite a bit

The speed of this specific function, measured with the described (micro) benchmark, improved, yes, no doubt. But that's not (by itself) relevant! If you want to use an optimized implementation of some code, it has to represent a significant improvement in the overall system as measured by meaningful, agreed-upon, high-level performance metrics.

If this function is called many times per user request, and the optimized version has a measurable impact on request latency, then sure, go for it! But, otherwise, making this kind of change is the textbook definition of premature optimization, never a good idea.

View on HN

When I see these kind of readability-decreasing optimizations in code reviews, I always push back. Almost always, that same Go code queries some other service over the network or loads data from the database, or similar, and nobody will ever notice the speed improvement of IndexOf over Split at the bottom line. But everyone working with that code at a later time will spend extra time (sometimes hours) just trying to understand what's going on. That's much more expensive than a few extra CPU cycles here and there.

Sure, if you're writing an inner loop for some heavy workload, e.g. in graphics or maybe parsing terabytes of JSON, it makes sense. But outside of these rarely occurring situations, you should always prefer readability over low-level optimizations.

OTOH note that this optimisation can be bundled into a helper function so it's not quite as bad as some others. In fact the helper function is in the stdlib: https://pkg.go.dev/strings#Cut

This is most useful for the "split once" version, which is genuinely improved:

    scoreWithColor := strings.Split(score, " ")
    scoreStr := scoreWithColor[0]
    color := scoreWithColor[1]

becomes

    scoreStr, color, _ := strings.Cut(score, " ")

The iteration is worse, but not to an irredeemable degree,

    rounds := strings.Split(game, "; ")

    isPossible := false
    for _, round := range rounds {
      red, green, blue := findScores(round)
      if isRoundPossible(red, green, blue) {
        isPossible = true
      } else {
        isPossible = false

        // If only one round is not possible, there is
        // no need to process the other ones.
        break
      }
    }

becomes:

    isPossible := false
    for {
      round, tail, hasNext := strings.Cut(game, ";")
      game = tail
      red, green, blue := findScores(round)
      if isRoundPossible(red, green, blue) {
        isPossible = true
      } else {
        isPossible = false

        // If only one round is not possible, there is
        // no need to process the other ones.
        break
      }
      if !hasNext {
        break
      }
    }

Reply on news.ycombinator.com