Address ConfigNode PR feedback

Address various PR feedback, including:
  * Remove unused map from ConfigNodeComplexValue
  * Stop caching KeyValue indexes in ConfigNodeComplexValue
  * Return an Iterable<Token> for the children() methods in
    ConfigNodeComplexValue and ConfigNodeKeyValue
  * Change all ConfigNode classes to implement AbstractConfigNode
  * Remove the constructor and the token instance variable
    from AbstractConfigNode. Make the render() method final and
    have it use a new tokens() method which returns the list
    of tokens contained by the node.
  * Stop caching values in ConfigNodeKeyValue

config/src/main/java/com/typesafe/config/impl/AbstractConfigNode.java

@@ -1,15 +1,16 @@
 package com.typesafe.config.impl;
 
 import com.typesafe.config.ConfigNode;
+import java.util.Collection;
 
 abstract class AbstractConfigNode implements ConfigNode {
-    final private Token token;
+    abstract Collection<Token> tokens();
-
+    final public String render() {
-    AbstractConfigNode(Token t) {
+        StringBuilder origText = new StringBuilder();
-        token = t;
+        Iterable<Token> tokens = tokens();
-    }
+        for (Token t : tokens) {
-
+            origText.append(t.tokenText());
-    public String render() {
+        }
-        return token.tokenText();
+        return origText.toString();
     }
 }

config/src/main/java/com/typesafe/config/impl/AbstractConfigNodeValue.java

@@ -0,0 +1,9 @@
+package com.typesafe.config.impl;
+
+// This is gross. We currently have a class that doesn't do anything, but is needed
+// to distinguish certain types of nodes from other types. This is required if we want
+// to be referencing the AbstractConfigNode class in implementation rather than the
+// ConfigNode interface, as we can't cast an AbstractConfigNode to an interface
+abstract class AbstractConfigNodeValue extends AbstractConfigNode {
+
+}

config/src/main/java/com/typesafe/config/impl/ConfigNodeBasic.java

@@ -1,7 +0,0 @@
-package com.typesafe.config.impl;
-
-final class ConfigNodeBasic extends AbstractConfigNode{
-    ConfigNodeBasic(Token t) {
-        super(t);
-    }
-}

config/src/main/java/com/typesafe/config/impl/ConfigNodeComplexValue.java

@@ -4,63 +4,54 @@ import com.typesafe.config.ConfigNode;
 
 import java.util.*;
 
-final class ConfigNodeComplexValue implements ConfigNode, ConfigNodeValue {
+final class ConfigNodeComplexValue extends AbstractConfigNodeValue {
-    private ArrayList<ConfigNode> children;
+    final private ArrayList<AbstractConfigNode> children;
-    final private LinkedHashMap<Path, Integer> map = new LinkedHashMap<>();
-    final ArrayList<Integer> keyValueIndexes;
 
-    ConfigNodeComplexValue(Collection<ConfigNode> children) {
+    ConfigNodeComplexValue(Collection<AbstractConfigNode> children) {
         this.children = new ArrayList(children);
-        keyValueIndexes = new ArrayList<Integer>();
-
-        // Construct the list of indexes of Key-Value nodes. Do this
-        // in reverse order, since all but the final duplicate will be removed.
-        for (int i = this.children.size() - 1; i >= 0; i--) {
-            ConfigNode currNode = this.children.get(i);
-            if (currNode instanceof ConfigNodeKeyValue) {
-                keyValueIndexes.add(i);
-            }
-        }
     }
 
-    public ArrayList<ConfigNode> children() {
+    public Iterable<AbstractConfigNode> children() {
         return children;
     }
 
-    public String render() {
+    protected Collection<Token> tokens() {
-        StringBuilder renderedText = new StringBuilder();
+        ArrayList<Token> tokens = new ArrayList();
-        for (ConfigNode child : children) {
+        for (AbstractConfigNode child : children) {
-            renderedText.append(child.render());
+            tokens.addAll(child.tokens());
         }
-        return renderedText.toString();
+        return tokens;
     }
 
-    protected ConfigNodeComplexValue changeValueOnPath(Path desiredPath, ConfigNodeValue value) {
+    protected ConfigNodeComplexValue changeValueOnPath(Path desiredPath, AbstractConfigNodeValue value) {
-        ArrayList<ConfigNode> childrenCopy = (ArrayList<ConfigNode>)(children.clone());
+        ArrayList<AbstractConfigNode> childrenCopy = new ArrayList(children);
         boolean replaced = value == null;
         ConfigNodeKeyValue node;
         Path key;
-        for (Integer keyValIndex : keyValueIndexes) {
+        for (int i = children.size() - 1; i >= 0; i--) {
-            node = (ConfigNodeKeyValue)children.get(keyValIndex.intValue());
+            if (!(children.get(i) instanceof ConfigNodeKeyValue)) {
+                continue;
+            }
+            node = (ConfigNodeKeyValue)children.get(i);
             key = Path.newPath(node.key().render());
             if (key.equals(desiredPath)) {
                 if (!replaced) {
-                    childrenCopy.set(keyValIndex.intValue(), node.replaceValue(value));
+                    childrenCopy.set(i, node.replaceValue(value));
                     replaced = true;
                 }
                 else
-                    childrenCopy.remove(keyValIndex.intValue());
+                    childrenCopy.remove(i);
             } else if (desiredPath.startsWith(key)) {
                 if (node.value() instanceof ConfigNodeComplexValue) {
                     Path remainingPath = desiredPath.subPath(key.length());
                     if (!replaced) {
                         node = node.replaceValue(((ConfigNodeComplexValue) node.value()).changeValueOnPath(remainingPath, value));
-                        if (node.render() != children.get(keyValIndex.intValue()).render())
+                        if (node.render() != children.get(i).render())
                             replaced = true;
-                        childrenCopy.set(keyValIndex.intValue(), node);
+                        childrenCopy.set(i, node);
                     } else {
                         node = node.replaceValue(((ConfigNodeComplexValue) node.value()).removeValueOnPath(remainingPath));
-                        childrenCopy.set(keyValIndex.intValue(), node);
+                        childrenCopy.set(i, node);
                     }
                 }
             }
@@ -68,20 +59,20 @@ final class ConfigNodeComplexValue implements ConfigNode, ConfigNodeValue {
         return new ConfigNodeComplexValue(childrenCopy);
     }
 
-    public ConfigNodeComplexValue setValueOnPath(Path desiredPath, ConfigNodeValue value) {
+    public ConfigNodeComplexValue setValueOnPath(Path desiredPath, AbstractConfigNodeValue value) {
         ConfigNodeComplexValue node = changeValueOnPath(desiredPath, value);
 
         // If the desired Path did not exist, add it
         if (node.render().equals(render())) {
-            ArrayList<ConfigNode> childrenCopy = (ArrayList<ConfigNode>)children.clone();
+            ArrayList<AbstractConfigNode> childrenCopy = new ArrayList<AbstractConfigNode>(children);
-            ArrayList<ConfigNode> newNodes = new ArrayList<ConfigNode>();
+            ArrayList<AbstractConfigNode> newNodes = new ArrayList();
-            newNodes.add(new ConfigNodeBasic(Tokens.newLine(null)));
+            newNodes.add(new ConfigNodeSingleToken(Tokens.newLine(null)));
             newNodes.add(new ConfigNodeKey(Tokens.newUnquotedText(null, desiredPath.render())));
-            newNodes.add(new ConfigNodeBasic(Tokens.newIgnoredWhitespace(null, " ")));
+            newNodes.add(new ConfigNodeSingleToken(Tokens.newIgnoredWhitespace(null, " ")));
-            newNodes.add(new ConfigNodeBasic(Tokens.COLON));
+            newNodes.add(new ConfigNodeSingleToken(Tokens.COLON));
-            newNodes.add(new ConfigNodeBasic(Tokens.newIgnoredWhitespace(null, " ")));
+            newNodes.add(new ConfigNodeSingleToken(Tokens.newIgnoredWhitespace(null, " ")));
             newNodes.add(value);
-            newNodes.add(new ConfigNodeBasic(Tokens.newLine(null)));
+            newNodes.add(new ConfigNodeSingleToken(Tokens.newLine(null)));
             childrenCopy.add(new ConfigNodeKeyValue(newNodes));
             node = new ConfigNodeComplexValue(childrenCopy);
         }

config/src/main/java/com/typesafe/config/impl/ConfigNodeKey.java

@@ -1,7 +1,15 @@
 package com.typesafe.config.impl;
 
+import java.util.Collection;
+import java.util.Collections;
+
 final class ConfigNodeKey extends AbstractConfigNode {
+    Token token;
     ConfigNodeKey(Token t) {
-        super(t);
+        token = t;
+    }
+
+    protected Collection<Token> tokens() {
+        return Collections.singletonList(token);
     }
 }

config/src/main/java/com/typesafe/config/impl/ConfigNodeKeyValue.java

@@ -1,48 +1,52 @@
 package com.typesafe.config.impl;
 
+import com.typesafe.config.ConfigException;
 import com.typesafe.config.ConfigNode;
 
 import java.util.ArrayList;
 import java.util.Collection;
 
-public class ConfigNodeKeyValue implements ConfigNode{
+public class ConfigNodeKeyValue extends AbstractConfigNode {
-    final private ArrayList<ConfigNode> children;
+    final private ArrayList<AbstractConfigNode> children;
-    private int configNodeValueIndex;
-    private ConfigNodeKey key;
-    private ConfigNodeValue value;
 
-    public ConfigNodeKeyValue(Collection<ConfigNode> children) {
+    public ConfigNodeKeyValue(Collection<AbstractConfigNode> children) {
-        this.children = new ArrayList<ConfigNode>(children);
+        this.children = new ArrayList(children);
-        for (int i = 0; i < this.children.size(); i++) {
+    }
-            ConfigNode currNode = this.children.get(i);
+
-            if (currNode instanceof ConfigNodeKey) {
+    protected Collection<Token> tokens() {
-                key = (ConfigNodeKey)currNode;
+        ArrayList<Token> tokens = new ArrayList();
-            } else if (currNode instanceof ConfigNodeValue) {
+        for (AbstractConfigNode child : children) {
-                value = (ConfigNodeValue)currNode;
+            tokens.addAll(child.tokens());
-                configNodeValueIndex = i;
+        }
+        return tokens;
+    }
+
+    public ConfigNodeKeyValue replaceValue(AbstractConfigNodeValue newValue) {
+        ArrayList<AbstractConfigNode> childrenCopy = new ArrayList(children);
+        for (int i = 0; i < childrenCopy.size(); i++) {
+            if (childrenCopy.get(i) instanceof AbstractConfigNodeValue) {
+                childrenCopy.set(i, newValue);
+                return new ConfigNodeKeyValue(childrenCopy);
             }
         }
+        throw new ConfigException.BugOrBroken("KeyValue node doesn't have a value");
     }
 
-    public String render() {
+    public AbstractConfigNodeValue value() {
-        StringBuilder renderedText = new StringBuilder();
+        for (int i = 0; i < children.size(); i++) {
-        for (ConfigNode child : children) {
+            if (children.get(i) instanceof AbstractConfigNodeValue) {
-            renderedText.append(child.render());
+                return (AbstractConfigNodeValue)children.get(i);
+            }
         }
-        return renderedText.toString();
+        throw new ConfigException.BugOrBroken("KeyValue node doesn't have a value");
-    }
-
-    public ConfigNodeKeyValue replaceValue(ConfigNodeValue newValue) {
-        ArrayList<ConfigNode> newChildren = (ArrayList<ConfigNode>)(children.clone());
-        newChildren.set(configNodeValueIndex, newValue);
-        return new ConfigNodeKeyValue(newChildren);
-    }
-
-    public ConfigNodeValue value() {
-        return value;
     }
 
     public ConfigNodeKey key() {
-        return key;
+        for (int i = 0; i < children.size(); i++) {
+            if (children.get(i) instanceof ConfigNodeKey) {
+                return (ConfigNodeKey)children.get(i);
+            }
+        }
+        throw new ConfigException.BugOrBroken("KeyValue node doesn't have a key");
     }
 }

config/src/main/java/com/typesafe/config/impl/ConfigNodeSimpleValue.java

@@ -1,10 +1,18 @@
 package com.typesafe.config.impl;
 
+import java.util.Collection;
+import java.util.Collections;
+
 /**
  * This class represents a leaf ConfigNode. This type of ConfigNode has no children.
  */
-class ConfigNodeSimpleValue extends AbstractConfigNode implements ConfigNodeValue {
+class ConfigNodeSimpleValue extends AbstractConfigNodeValue {
+    Token token;
     ConfigNodeSimpleValue(Token value) {
-        super(value);
+        token = value;
+    }
+
+    protected Collection<Token> tokens() {
+        return Collections.singletonList(token);
     }
 }

config/src/main/java/com/typesafe/config/impl/ConfigNodeSingleToken.java

@@ -0,0 +1,15 @@
+package com.typesafe.config.impl;
+
+import java.util.Collection;
+import java.util.Collections;
+
+final class ConfigNodeSingleToken extends AbstractConfigNode{
+    Token token;
+    ConfigNodeSingleToken(Token t) {
+        token = t;
+    }
+
+    protected Collection<Token> tokens() {
+        return Collections.singletonList(token);
+    }
+}

config/src/main/java/com/typesafe/config/impl/ConfigNodeValue.java

@@ -1,7 +0,0 @@
-package com.typesafe.config.impl;
-
-import com.typesafe.config.ConfigNode;
-
-public interface ConfigNodeValue extends ConfigNode {
-    public String render();
-}

config/src/test/scala/com/typesafe/config/impl/ConfigNodeTest.scala

@@ -21,7 +21,7 @@ class ConfigNodeTest extends TestUtils {
         assertEquals(node.render(), token.tokenText())
     }
 
-    private def keyValueNodeTest(key: ConfigNodeKey, value: ConfigNodeValue, trailingWhitespace: ConfigNodeBasic, newValue: ConfigNodeValue) {
+    private def keyValueNodeTest(key: ConfigNodeKey, value: AbstractConfigNodeValue, trailingWhitespace: ConfigNodeSingleToken, newValue: AbstractConfigNodeValue) {
         val keyValNode = nodeKeyValuePair(key, value, trailingWhitespace)
         assertEquals(key.render() + " : " + value.render() + trailingWhitespace.render(), keyValNode.render())
         assertEquals(key.render, keyValNode.key().render())
@@ -32,7 +32,7 @@ class ConfigNodeTest extends TestUtils {
         assertEquals(newValue.render(), newKeyValNode.value().render())
     }
 
-    private def topLevelValueReplaceTest(value: ConfigNodeValue, newValue: ConfigNodeValue, key: Token = tokenString("foo")) {
+    private def topLevelValueReplaceTest(value: AbstractConfigNodeValue, newValue: AbstractConfigNodeValue, key: Token = tokenString("foo")) {
         val complexNodeChildren = List(nodeOpenBrace,
                                        nodeKeyValuePair(nodeWhitespace("       "), configNodeKey(key),value, nodeWhitespace("    ")),
                                        nodeCloseBrace)
@@ -45,7 +45,7 @@ class ConfigNodeTest extends TestUtils {
         assertEquals(finalText, newNode.render())
     }
 
-    private def replaceDuplicatesTest(value1: ConfigNodeValue, value2: ConfigNodeValue, value3: ConfigNodeValue) {
+    private def replaceDuplicatesTest(value1: AbstractConfigNodeValue, value2: AbstractConfigNodeValue, value3: AbstractConfigNodeValue) {
         val key = nodeUnquotedKey("foo")
         val keyValPair1 = nodeKeyValuePair(key, value1)
         val keyValPair2 = nodeKeyValuePair(key, value2)
@@ -58,7 +58,7 @@ class ConfigNodeTest extends TestUtils {
         assertEquals(finalText, complexNode.setValueOnPath(Path.newPath("foo"), nodeInt(15)).render())
     }
 
-    private def nonExistentPathTest(value: ConfigNodeValue) {
+    private def nonExistentPathTest(value: AbstractConfigNodeValue) {
         val node = configNodeComplexValue(List(nodeKeyValuePair(nodeUnquotedKey("bar"), nodeInt(15))))
         assertEquals("bar : 15", node.render())
         val newNode = node.setValueOnPath(Path.newPath("foo"), value)

config/src/test/scala/com/typesafe/config/impl/TestUtils.scala

@@ -672,33 +672,33 @@ abstract trait TestUtils {
     }
 
     def configNodeBasic(value: Token) = {
-        new ConfigNodeBasic(value: Token)
+        new ConfigNodeSingleToken(value: Token)
     }
 
-    def configNodeComplexValue(nodes: List[ConfigNode]) = {
+    def configNodeComplexValue(nodes: List[AbstractConfigNode]) = {
         new ConfigNodeComplexValue(nodes.asJavaCollection)
     }
 
-    def nodeColon = new ConfigNodeBasic(Tokens.COLON)
+    def nodeColon = new ConfigNodeSingleToken(Tokens.COLON)
-    def nodeSpace = new ConfigNodeBasic(tokenUnquoted(" "))
+    def nodeSpace = new ConfigNodeSingleToken(tokenUnquoted(" "))
-    def nodeOpenBrace = new ConfigNodeBasic(Tokens.OPEN_CURLY)
+    def nodeOpenBrace = new ConfigNodeSingleToken(Tokens.OPEN_CURLY)
-    def nodeCloseBrace = new ConfigNodeBasic(Tokens.CLOSE_CURLY)
+    def nodeCloseBrace = new ConfigNodeSingleToken(Tokens.CLOSE_CURLY)
-    def nodeOpenBracket = new ConfigNodeBasic(Tokens.OPEN_SQUARE)
+    def nodeOpenBracket = new ConfigNodeSingleToken(Tokens.OPEN_SQUARE)
-    def nodeCloseBracket = new ConfigNodeBasic(Tokens.CLOSE_SQUARE)
+    def nodeCloseBracket = new ConfigNodeSingleToken(Tokens.CLOSE_SQUARE)
-    def nodeComma = new ConfigNodeBasic(Tokens.COMMA)
+    def nodeComma = new ConfigNodeSingleToken(Tokens.COMMA)
-    def nodeLine(line: Integer) = new ConfigNodeBasic(tokenLine(line))
+    def nodeLine(line: Integer) = new ConfigNodeSingleToken(tokenLine(line))
-    def nodeWhitespace(whitespace: String) = new ConfigNodeBasic(tokenWhitespace(whitespace))
+    def nodeWhitespace(whitespace: String) = new ConfigNodeSingleToken(tokenWhitespace(whitespace))
     def nodeQuotedKey(key: String) = configNodeKey(tokenString(key))
     def nodeUnquotedKey(key: String) = configNodeKey(tokenUnquoted(key))
-    def nodeKeyValuePair(key: ConfigNodeKey, value: ConfigNodeValue) = {
+    def nodeKeyValuePair(key: ConfigNodeKey, value: AbstractConfigNodeValue) = {
         val nodes = List(key, nodeSpace, nodeColon, nodeSpace, value)
         new ConfigNodeKeyValue(nodes.asJavaCollection)
     }
-    def nodeKeyValuePair(key: ConfigNodeKey, value: ConfigNodeValue, trailingWhitespace: ConfigNodeBasic) = {
+    def nodeKeyValuePair(key: ConfigNodeKey, value: AbstractConfigNodeValue, trailingWhitespace: ConfigNodeSingleToken) = {
         val nodes = List(key, nodeSpace, nodeColon, nodeSpace, value, trailingWhitespace)
         new ConfigNodeKeyValue(nodes.asJavaCollection)
     }
-    def nodeKeyValuePair(leadingWhitespace: ConfigNodeBasic, key: ConfigNodeKey, value: ConfigNodeValue, trailingWhitespace: ConfigNodeBasic) = {
+    def nodeKeyValuePair(leadingWhitespace: ConfigNodeSingleToken, key: ConfigNodeKey, value: AbstractConfigNodeValue, trailingWhitespace: ConfigNodeSingleToken) = {
         val nodes = List(leadingWhitespace, key, nodeSpace, nodeColon, nodeSpace, value, trailingWhitespace)
         new ConfigNodeKeyValue(nodes.asJavaCollection)
     }
@@ -708,8 +708,8 @@ abstract trait TestUtils {
     def nodeDouble(value: Double) = new ConfigNodeSimpleValue(tokenDouble(value))
     def nodeTrue = new ConfigNodeSimpleValue(tokenTrue)
     def nodeFalse = new ConfigNodeSimpleValue(tokenFalse)
-    def nodeCommentHash(text: String) = new ConfigNodeBasic(tokenCommentHash(text))
+    def nodeCommentHash(text: String) = new ConfigNodeSingleToken(tokenCommentHash(text))
-    def nodeCommentDoubleSlash(text: String) = new ConfigNodeBasic(tokenCommentDoubleSlash(text))
+    def nodeCommentDoubleSlash(text: String) = new ConfigNodeSingleToken(tokenCommentDoubleSlash(text))
     def nodeUnquotedText(text: String) = new ConfigNodeSimpleValue(tokenUnquoted(text))
     def nodeNull = new ConfigNodeSimpleValue(tokenNull)
     def nodeKeySubstitution(s: String) = new ConfigNodeSimpleValue(tokenKeySubstitution(s))