Best practice examples in Effective Go

[Ben Hoyt]

I'm not entirely sure this is the right group, but I have a couple of comments about the code example in the "Interfaces" section of "Effective Go" (https://golang.org/doc/effective_go.html#interfaces):

    type Sequence []int

    // ... Len, Less, and Swap methods ...

    // Method for printing - sorts the elements before printing.

    func (s Sequence) String() string {

        sort.Sort(s)

        str := "["

        for i, elem := range s {

            if i > 0 {

                str += " "

            }

            str += fmt.Sprint(elem)

        }

        return str + "]"

    }

It seems to me there are a couple of things that aren't good habits in this example, and if that's the case, that we shouldn't be showing them as examples in *Effective* Go:

1) Having a String() method modify the object (by sorting it in place). There's not actually a strict contract that disallows that, but I would definitely not expect that a String() method would modify the object under my feet. If it it's going to format it sorted, it should make a copy of the slice and sort the copy.

2) Appending to a string over and over in a loop. I'm not 100% sure how clever the compiler is, but won't this allocate a new string and copy the bytes each time around the loop, leading to O(N^2) behavior? (Quick test showing N^2 behaviour: https://play.golang.org/p/98e9uMsC384) I usually try to avoid appending to a string inside a loop like this for that reason, and instead append() to a slice of strings or write to a bytes buffer.

Thoughts?

-Ben

[Brad Fitzpatrick]

Both good points.

Could you file a documentation bug at https://golang.org/issues/new ?

--
You received this message because you are subscribed to the Google Groups "golang-dev" group.
To unsubscribe from this group and stop receiving emails from it, send an email to golang-dev+...@googlegroups.com.
For more options, visit https://groups.google.com/d/optout.

[Robert Engels]

Can’t the compiler replace the string concatenation with a StringBuilder/buffer behind the scenes? This is what is done in java and it allows for simple code like the example to be used with no performance impact.

[Ben Hoyt]

Thanks! Issue opened: https://github.com/golang/go/issues/28773

[Brad Fitzpatrick]

The compiler does not, and likely never will. The cost to detect that pattern would likely not pay for itself (would not make the compiler faster itself to offset the detection cost), and there would probably be arguments that it'd only encourage bad code in the wild. (i.e. the compiler's job is not to make algorithmically bad code better.)

[Robert Engels]

I am not sure you can call it algorithmically bad code. The reason the compiler can do it, and does in java is two fold, that strings are immutable, and escape analysis. Imagine that this code called other methods in the loop passing the string - this gets much more difficult when you are instead working with a raw buffer or slice of strings... the compiler doing escape analysis and in-lining can make string processing loops far more efficient in a GC language, so you work with strings, not lower level constructs. 

[Kyle Wood]

Java actually only does that for chained concatenations, i.e. String thing = "some" + i + "text"; It does not detect or use builders for concatenations in a loop, and it is considered bad practice to concatenate in a loop for the same reason as it is in Go. That pattern would probably be nearly impossible to properly handle.

[Robert Engels]

That is kind of true. There is more to it. When using the + the compiler will generate synthetic methods when constants are involved. See http://openjdk.java.net/jeps/280

Also, the fact that it doesn’t work in loops is a bummer, but I’m not convinced given the efforts on optimizing string concatenation happening that this will always be the case. But yes, right now loops require using manual buffer management.