One of the termui tests was failing; this fixes the numbered.Less() function.

Something in sorting was broken at some point, maybe during a Go version
upgrade? That code hasn't changed in a while. While fixing the code, I rewrote
the `numbered.Less()` function. It's shorter, and I think it's clearer, but in
addition to being correct now it's also about 10% faster than the original.

termui/linegraph.go

@@ -171,57 +171,63 @@ type numbered []string
 
 func (n numbered) Len() int      { return len(n) }
 func (n numbered) Swap(i, j int) { n[i], n[j] = n[j], n[i] }
+
 func (n numbered) Less(i, j int) bool {
-	a := n[i]
+	ars := n[i]
-	b := n[j]
+	brs := n[j]
-	for i := 0; i < len(a); i++ {
+	// iterate over the runes in string A
-		ac := a[i]
+	var ai int
-		if unicode.IsDigit(rune(ac)) {
+	for ai = 0; ai < len(ars); ai++ {
-			j := i + 1
+		ar := ars[ai]
-			for ; j < len(a); j++ {
+		// if brs has been equal to ars so far, but is shorter, than brs is less
-				if !unicode.IsDigit(rune(a[j])) {
+		if ai >= len(brs) {
-					break
+			return false
-				}
+		}
-				if j >= len(b) {
+		br := brs[ai]
-					return false
+		// handle numbers if we find them
-				}
+		if unicode.IsDigit(rune(ar)) {
-				if !unicode.IsDigit(rune(b[j])) {
+			// if ar is a digit and br isn't, then ars is less
-					return false
+			if !unicode.IsDigit(rune(brs[ai])) {
-				}
+				return true
 			}
-			an, err := strconv.Atoi(a[i:j])
+			// now get the range for the full numbers, which is the sequence of consecutive digits from each
+			ae := ai + 1
+			be := ae
+			for ; ae < len(ars) && unicode.IsDigit(rune(ars[ae])); ae++ {
+			}
+			for ; be < len(brs) && unicode.IsDigit(rune(brs[be])); be++ {
+			}
+			if be < ae {
+				return false
+			}
+			adigs, err := strconv.Atoi(string(ars[ai:ae]))
 			if err != nil {
 				return true
 			}
-			if j > len(b) {
+			bdigs, err := strconv.Atoi(string(brs[ai:be]))
-				return false
-			}
-			for ; j < len(b); j++ {
-				if !unicode.IsDigit(rune(b[j])) {
-					break
-				}
-			}
-			bn, err := strconv.Atoi(b[i:j])
 			if err != nil {
 				return true
 			}
-			if an < bn {
+			// The numbers are equal; continue with the rest of the string
-				return true
+			if adigs == bdigs {
-			} else if bn < an {
+				ai = ae - 1
-				return false
+				continue
 			}
-			i = j
+			return adigs < bdigs
-		}
+		} else if unicode.IsDigit(rune(br)) {
-		if i >= len(a) {
-			return true
-		} else if i >= len(b) {
 			return false
 		}
-		if ac < b[i] {
+		if ar == br {
-			return true
+			continue
-		} else if b[i] < ac {
-			return false
 		}
+		// No digits, so compare letters
+		if ar < br {
+			return true
+		}
+		return false
 	}
-	return true
+	if ai <= len(brs) {
+		return true
+	}
+	return false
 }

termui/linegraph_test.go

@@ -1,7 +1,10 @@
 package termui
 
-import "testing"
+import (
-import "sort"
+	"github.com/stretchr/testify/assert"
+	"sort"
+	"testing"
+)
 
 func TestLess(t *testing.T) {
 	tests := []struct {
@@ -16,6 +19,10 @@ func TestLess(t *testing.T) {
 		{a: "a2", b: "2", e: false},
 		{a: "a2", b: "a10", e: true},
 		{a: "a20", b: "a2", e: false},
+		{a: "abc", b: "abc20", e: true},
+		{a: "a1", b: "abc", e: true},
+		{a: "a20", b: "abc", e: true},
+		{a: "abc20", b: "abc", e: false},
 		{a: "abc20", b: "def2", e: true},
 		{a: "abc20", b: "abc2", e: false},
 		{a: "abc20", b: "abc20", e: true},
@@ -29,9 +36,7 @@ func TestLess(t *testing.T) {
 	for _, k := range tests {
 		n := numbered([]string{k.a, k.b})
 		g := n.Less(0, 1)
-		if g != k.e {
+		assert.Equal(t, k.e, g, "%s < %s %t", k.a, k.b, g)
-			t.Errorf("%s < %s: expected %v, got %v", k.a, k.b, k.e, g)
-		}
 	}
 }
 
@@ -40,7 +45,7 @@ func TestSort(t *testing.T) {
 		in, ex numbered
 	}{
 		{
-			in: numbered{"abc", "def", "abc", "abc", "def", "abc", "1", "10", "1", "2", "a2", "2", "a2", "a10", "a20", "a2", "abc20", "def2", "abc20", "abc2", "abc20", "abc20", "abc30", "abc20", "abc20a", "abc20", "abc20", "abc20a", "abc20", "abc2a"},
+			in: numbered{"abc", "def", "abc", "def", "abc", "1", "10", "1", "2", "a2", "2", "a2", "a10", "a20", "a2", "abc20", "def2", "abc20", "abc2", "abc20", "abc20", "abc30", "abc20", "abc20a", "abc20", "abc20", "abc20a", "abc20", "abc2a", "abc"},
 			ex: numbered{"1", "1", "2", "2", "10", "a2", "a2", "a2", "a10", "a20", "abc", "abc", "abc", "abc", "abc2", "abc2a", "abc20", "abc20", "abc20", "abc20", "abc20", "abc20", "abc20", "abc20", "abc20a", "abc20a", "abc30", "def", "def", "def2"},
 		},
 		{
@@ -51,10 +56,37 @@ func TestSort(t *testing.T) {
 
 	for _, k := range tests {
 		sort.Sort(k.in)
-		for i, v := range k.in {
+		assert.Equal(t, k.ex, k.in)
-			if v != k.ex[i] {
+	}
-				t.Errorf("failed to properly sort\n\texpected: %v\n\tgot:      %v", k.ex, k.in)
+}
-			}
+
+func BenchmarkLess(b *testing.B) {
+	tests := []numbered{
+		numbered([]string{"abc", "def"}),
+		numbered([]string{"abc", "abc"}),
+		numbered([]string{"def", "abc"}),
+		numbered([]string{"1", "10"}),
+		numbered([]string{"1", "2"}),
+		numbered([]string{"a2", "2"}),
+		numbered([]string{"a2", "a10"}),
+		numbered([]string{"a20", "a2"}),
+		numbered([]string{"abc", "abc20"}),
+		numbered([]string{"a1", "abc"}),
+		numbered([]string{"a20", "abc"}),
+		numbered([]string{"abc20", "abc"}),
+		numbered([]string{"abc20", "def2"}),
+		numbered([]string{"abc20", "abc2"}),
+		numbered([]string{"abc20", "abc20"}),
+		numbered([]string{"abc30", "abc20"}),
+		numbered([]string{"abc20a", "abc20"}),
+		numbered([]string{"abc20", "abc20a"}),
+		numbered([]string{"abc20", "abc2a"}),
+		numbered([]string{"abc20", "abc3a"}),
+		numbered([]string{"abc20", "abc2abc"}),
+	}
+	for i := 0; i < b.N; i++ {
+		for _, t := range tests {
+			t.Less(0, 1)
 		}
 	}
 }