code review 223096: explode(): a little efficient improved.

explode(): a little efficient improved.

[MC.Spring, 2010-03-02 09:49:36]

Hello golang-dev@googlegroups.com (cc: golang-dev@googlegroups.com),

I'd like you to review this change.

[gri, 2010-03-02 18:42:49]

http://codereview.appspot.com/223096/diff/4/1001
File src/pkg/strings/strings.go (right):

http://codereview.appspot.com/223096/diff/4/1001#newcode15
src/pkg/strings/strings.go:15: func explode(s string, n int) []string {
I don't believe this is faster. You call RuneCountInString which effectively
needs to decode the entire string. Thus, this version does the string-to-rune
decoding twice instead of once.

It's often hard to make efficiency claims without measurements. If you are
convinced this is better, please provide measurements.

http://codereview.appspot.com/223096/diff/4/1001#newcode40
src/pkg/strings/strings.go:40: if len(sep) == 1 {
For symmetry with the functions below, perhaps introduce n := len(sep) as below,
and do tests the same way. Need to rename the existing n into count, for
instance.

[rsc1, 2010-03-02 18:45:54]

please don't rename a function parameter just to use it as
a local variable.  the function parameter names are chosen for
ease of documentation.  local variables take second priority.

russ

[MC.Spring, 2010-03-03 13:56:43]

Hello golang-dev@googlegroups.com, gri (cc: golang-dev@googlegroups.com),

Please take another look.

[gri, 2010-03-03 18:41:44]

Again, you need to fill out the CLA (see http://golang.org/doc/contribute.html
). Without your consent I can not submit this under your name, once it looks
good.

Thanks.
- gri

http://codereview.appspot.com/223096/diff/6/1006
File src/pkg/strings/strings.go (right):

http://codereview.appspot.com/223096/diff/6/1006#newcode16
src/pkg/strings/strings.go:16: l := utf8.RuneCountInString(s)
Again, I would not change this function. RuneCountToString needs to decode the
entire string to count the nunber of runes. Thus, now this function needs to
decode the string twice as opposed to just once in the original. Please revert
these changes.

http://codereview.appspot.com/223096/diff/6/1006#newcode35
src/pkg/strings/strings.go:35: if sep == "" {
Now that we have the l, really keep the code symmetric with all the other
functions:

l := len(sep)
if l == 0 {
   return ...
}

Admittedly, this is just cosmetic, but it's nice to see the symmetry in all
these routines as it will make easier to read.

http://codereview.appspot.com/223096/diff/6/1006#newcode39
src/pkg/strings/strings.go:39: l := len(sep)
move this up to the top as suggested

[rsc, 2010-03-03 21:38:43]

And there's still no benchmark.
I'd like to see some measurement that shows these changes matter.
The code is getting more complex; the new complexity
has to be justified by an actual improvement.

Russ

[MC.Spring, 2010-03-04 03:29:16]

Hello golang-dev@googlegroups.com, gri (cc: golang-dev@googlegroups.com),

Please take another look.

[MC.Spring, 2010-03-04 05:12:44]

Hello golang-dev@googlegroups.com, gri (cc: golang-dev@googlegroups.com),

Please take another look.

[rsc, 2010-03-05 00:12:05]

LGTM

thanks

[rsc, 2010-03-05 00:16:00]

*** Submitted as http://code.google.com/p/go/source/detail?r=bdff86679d85 ***

strings: make Split(s, "", n) faster

R=rsc
CC=golang-dev
http://codereview.appspot.com/223096

Committer: Russ Cox <rsc@golang.org>