Implement iterator (#1)

* Implement iterator

* .

Author: opethe1st

iterator.go

@@ -0,0 +1,19 @@
+package json
+
+
+type iterator struct{
+	s string
+	offset int
+}
+
+func (iter *iterator) getCurrent() byte {
+	return iter.s[iter.offset]
+}
+
+func (iter *iterator) advance() {
+	iter.offset++
+}
+
+func (iter *iterator) isEnd() bool {
+	return iter.offset >= len(iter.s)
+}

load.go

@@ -14,106 +14,96 @@ import (
 
 // Load is used load an object from a string
 func Load(s string) interface{} {
-	_, value := load(s, 0)
-	return value
+	iter := &iterator{s:s}
+	return load(iter)
 }
 
-func load(s string, current int) (int, interface{}) {
-	current = consumeWhiteSpace(s, current)
+
+func load(iter *iterator)  interface{} {
+	consumeWhiteSpace(iter)
 	switch {
-	case isString(s, current):
-		return loadString(s, current)
-	case isSequence(s, current):
-		return loadSequence(s, current)
-	case isMapping(s, current):
-		return loadMapping(s, current)
+	case iter.getCurrent() == '"':
+		return loadString(iter)
+	case iter.getCurrent() == '[':
+		return loadSequence(iter)
+	case iter.getCurrent() == '{':
+		return loadMapping(iter)
 	default:
-		end := current + 100
-		if len(s) < end {
-			end = len(s)
+		end := iter.offset + 100
+		if len(iter.s) < end {
+			end = len(iter.s)
 		}
-		panic(fmt.Sprintf("There is an error around\n\n%s", s[current:end]))
+		panic(fmt.Sprintf("There is an error around\n\n%s", iter.s[iter.offset:end]))
 	}
 }
 
 // strings
 
-func isString(s string, current int) bool {
-	return s[current] == '"'
-}
-
-func loadString(s string, current int) (int, interface{}) {
-	// actually should probably raise an error if '"' isn't consumed
-	start := consume(s, current, '"')
-	current = start
-	for current < len(s) && s[current] != '"' {
-		current++
+func loadString(iter *iterator) interface{} {
+	consume(iter, '"')
+	start := iter.offset
+	for !iter.isEnd() && (iter.getCurrent() != '"') {
+		iter.advance()
 	}
-	// and current + 1 since the next not visited character in s is at current + 1
-	return current + 1, s[start:current]
+	end := iter.offset
+	consume(iter, '"')
+	return iter.s[start:end]
 }
 
 // sequences
 
-func isSequence(s string, current int) bool {
-	return s[current] == '['
-}
-
-func loadSequence(s string, current int) (int, []interface{}) {
+func loadSequence(iter *iterator)  []interface{} {
 	seq := make([]interface{}, 0)
-	// actually should probably raise an error if '[' isn't consumed
-	current = consume(s, current, '[')
+	consume(iter, '[')
 	var item interface{}
-	for (current < len(s)) && (s[current] != ']') {
-		current, item = load(s, current)
-		current = consumeWhiteSpace(s, current)
-		// technically not allowed to have ["key",] but it is currently allowed
-		current = consume(s, current, ',')
-		current = consumeWhiteSpace(s, current)
+	for !iter.isEnd() && (iter.getCurrent() != ']') {
+		item = load(iter)
 		seq = append(seq, item)
-		current = consumeWhiteSpace(s, current)
+		consumeWhiteSpace(iter)
+		if iter.getCurrent() == ']'{
+			break
+		}
+		consume(iter, ',')
+		consumeWhiteSpace(iter)
 	}
-	return current + 1, seq
+	consume(iter, ']')
+	return seq
 }
 
 // mappings
 
-func isMapping(s string, current int) bool {
-	return s[current] == '{'
-}
-
-func loadMapping(s string, current int) (int, map[string]interface{}) {
+func loadMapping(iter *iterator) map[string]interface{} {
 	mapping := make(map[string]interface{}, 0)
-	// actually should probably raise an error if '{' isn't consumed
-	current = consume(s, current, '{')
+	consume(iter, '{')
 	var key, value interface{}
-	for (current < len(s)) && (s[current] != '}') {
-		current, key = load(s, current)
-		current = consumeWhiteSpace(s, current)
-		current = consume(s, current, ':')
-		current = consumeWhiteSpace(s, current)
-		current, value = load(s, current)
+	for !iter.isEnd() && (iter.s[iter.offset] != '}')  {
+		key = load(iter)
+		consumeWhiteSpace(iter)
+		consume(iter, ':')
+		consumeWhiteSpace(iter)
+		value = load(iter)
 		mapping[key.(string)] = value
-		// technically not allowed to have {"key":"value",} but it is currently allowed
-		current = consume(s, current, ',')
-		current = consumeWhiteSpace(s, current)
+		if iter.getCurrent() == '}'{
+			break
+		}
+		consume(iter, ',')
+		consumeWhiteSpace(iter)
 	}
-	return current + 1, mapping
+	consume(iter, '}')
+	return mapping
 }
 
 // utils - this could be in a separate file
 
-// consumeWhiteSpace returns the next index after current such that s[index] is not whitespace
-func consumeWhiteSpace(s string, current int) int {
-	for current < len(s) && unicode.IsSpace(rune(s[current])) {
-		current++
+func consumeWhiteSpace(iter *iterator){
+	for !iter.isEnd() && unicode.IsSpace(rune(iter.getCurrent())) {
+		iter.advance()
 	}
-	return current
 }
 
-func consume(s string, current int, char byte) int {
-	if s[current] == char {
-		return current + 1
+func consume(iter *iterator, char byte) {
+	// actually should probably raise an error if char isn't consumed
+	if iter.getCurrent() == char {
+		iter.advance()
 	}
-	return current
 }

load_test.go

@@ -36,21 +36,10 @@ func TestLoad(t *testing.T) {
 
 	for _, testcase := range testCases {
 		output := Load(testcase.input)
-		assert.Equal(output, testcase.expectedOutput, "These should be equal")
+		assert.Equal(testcase.expectedOutput, output)
 	}
 }
 
-func TestIsString(t *testing.T) {
-	testCases := []TestCase{
-		{`This is a string`, false},
-		{`"Key"`, true},
-	}
-	for _, testcase := range testCases {
-		if output := isString(testcase.input, 0); output != testcase.expectedOutput {
-			t.Errorf("Expected isString (%v) to be %t but got %t", testcase.input, testcase.expectedOutput, output)
-		}
-	}
-}
 
 func TestLoadString(t *testing.T) {
 	testCases := []TestCase{
@@ -60,8 +49,9 @@ func TestLoadString(t *testing.T) {
 		{`"Key"   `, "Key"},
 	}
 	for _, testcase := range testCases {
-		if _, output := loadString(testcase.input, 0); output != testcase.expectedOutput {
-			t.Errorf("Expected loadString (%v) to be %v but got %v", testcase.input, testcase.expectedOutput, output)
+		iter := &iterator{s:testcase.input}
+		if output := loadString(iter); output != testcase.expectedOutput {
+			t.Errorf("Expected loadString(%v) to be %v but got %v", iter, testcase.expectedOutput, output)
 		}
 	}
 
@@ -71,23 +61,13 @@ func TestLoadString(t *testing.T) {
 		{`"Key"   `, 5},
 	}
 	for _, testcase := range testCases {
-		if output, _ := loadString(testcase.input, 0); output != testcase.expectedOutput {
-			t.Errorf("Expected loadString (%v) to be %v but got %v", testcase.input, testcase.expectedOutput, output)
+		iter := &iterator{s:testcase.input}
+		if loadString(iter); iter.offset != testcase.expectedOutput {
+			t.Errorf("Expected loadString(%v) to be %v but got %v", iter, testcase.expectedOutput, iter.offset)
 		}
 	}
 }
 
-func TestIsSequence(t *testing.T) {
-	testCases := []TestCase{
-		{`[]`, true},
-		{`"Key"`, false},
-	}
-	for _, testcase := range testCases {
-		if output := isSequence(testcase.input, 0); output != testcase.expectedOutput {
-			t.Errorf("Expected isSequence (%v) to be %t but got %t", testcase.input, testcase.expectedOutput, output)
-		}
-	}
-}
 
 func TestLoadSequence(t *testing.T) {
 	assert := assert.New(t)
@@ -100,20 +80,9 @@ func TestLoadSequence(t *testing.T) {
 		{`["v1", {"v2": "v3"}]`, []interface{}{"v1", map[string]interface{}{"v2": "v3"}}},
 	}
 	for _, testcase := range testCases {
-		_, output := loadSequence(testcase.input, 0)
-		assert.Equal(output, testcase.expectedOutput, "Expected loadSequence(%v) to be %v but got %v", testcase.input, testcase.expectedOutput, output)
-	}
-}
-
-func TestIsMapping(t *testing.T) {
-	testCases := []TestCase{
-		{`[]`, false},
-		{`{`, true},
-	}
-	for _, testcase := range testCases {
-		if output := isMapping(testcase.input, 0); output != testcase.expectedOutput {
-			t.Errorf("Expected isSequence (%v) to be %t but got %t", testcase.input, testcase.expectedOutput, output)
-		}
+		iter := &iterator{s:testcase.input}
+		output := loadSequence(iter)
+		assert.Equal(testcase.expectedOutput, output,  "Expected loadSequence(%v) to be %v but got %v", iter, testcase.expectedOutput, output)
 	}
 }
 
@@ -126,20 +95,22 @@ func TestLoadMapping(t *testing.T) {
 		{`{"v1": ["v2", "v3"]}`, map[string]interface{}{"v1": []interface{}{"v2", "v3"}}},
 	}
 	for _, testcase := range testCases {
-		_, output := loadMapping(testcase.input, 0)
-		assert.Equal(output, testcase.expectedOutput, "Expected loadMapping(%v) to be %v but got %v", testcase.input, testcase.expectedOutput, output)
+		iter := &iterator{s:testcase.input}
+		output := loadMapping(iter)
+		assert.Equal(testcase.expectedOutput, output, "Expected loadMapping(%v) to be %v but got %v", iter, testcase.expectedOutput, output)
 	}
 }
 
 func TestConsumeSpaces(t *testing.T) {
+	assert := assert.New(t)
 	testCases := []TestCase{
 		{`"key"`, 0},
 		{` "key" `, 1},
 		{`   "key"`, 3},
 	}
 	for _, testcase := range testCases {
-		if output := consumeWhiteSpace(testcase.input, 0); output != testcase.expectedOutput {
-			t.Errorf("Expected consumeWhiteSpace (%v) to be %d but got %d", testcase.input, testcase.expectedOutput, output)
-		}
+		iter := &iterator{s:testcase.input}
+		consumeWhiteSpace(iter)
+		assert.Equal(testcase.expectedOutput, iter.offset, "Expected consumeWhiteSpace(%v) to be %d but got %d", iter, testcase.expectedOutput, iter.offset)
 	}
 }

log.md

@@ -0,0 +1,40 @@
+## Learnings
+
+I learned that Go's "=" is not good enough for asserting equality of nested objects when writing tests. This was disappointing because it meant I needed to
+use a third party library.
+I learned that you need to be careful about using ":=" in a loop if it shadows an outer variable. I lost a couple of hours to trying to debug this
+
+P.s I skimmed through the official implementation and it feels like I could do a better job (Is it hard to follow or it is just me?)
+Plus lots of state. My implementation probably suffers from using recursion though
+
+
+### Random thoughts
+Not use recursion? - is that possible?
+
+After looking at the code, I realize that instead of passing (s, current) into virtually every function, I could create a scanner type
+and have these functions be methods in this type.
+```golang
+type Scanner struct{
+    s string
+    current int
+}
+
+func (s *Scanner) consumeWhiteSpace(){
+
+}
+
+func (s *Scanner) isMappingStart(){
+
+}
+
+func (s *scanner) isMappingEnd(){
+
+}
+```
+Other thing is to introduce the concept of tokens - only needed for strings? Since that's where you can have escaped characters!
+
+So initially, I introduced the concept of scanners but then I realized that it didn't fit completely.
+So I discovered the concept that was missing was the concept of iterators. A iterator is simply - struct{s string, offset int}
+And this made other stuff make sense since I moved the stuff that didn't make sense in the iterator to standalone functions that instead of (s, current)
+made use of the iterator. The other interesting thing is that I violated Command Query Responsibility Segregation (CQRS) and it was the right decision.
+Know when to break the rules!