garydgregory commented on a change in pull request #188:
URL:
https://github.com/apache/commons-collections/pull/188#discussion_r499084931
##########
File path: src/main/java/org/apache/commons/collections4/MapBuilder.java
##########
@@ -0,0 +1,115 @@
+package org.apache.commons.collections4;
+
+import org.apache.commons.collections4.map.HashedMap;
+
+import java.util.*;
+
+/**
+ * Defines an Helper Builder that generates a {@code Map}
+ * A Builder class to help decide which type of map to use based on simple
requirements.
+ * Currently It takes care of only basic types of Maps and can be extended to
different types of Maps available in the ecosystem.
+ *
+ * <pre>{@code
+ * Map builderMap = new
MapBuilder().setIterationOrder(MapBuilder.KeyOrder.INSERTION_ORDER).build();
+ * builderMap.put("A", 1);
+ * builderMap.put("X", 24);
+ * builderMap.put("B", 2);
+ * builderMap.put("Y", 26);
+ * }</pre>
+ *
+ * @author Amita Pradhan
+ */
+public class MapBuilder {
+
+ private Comparator comparator;
+ private KeyOrder iterationOrder;
+ private boolean synchronizedMap;
+ private boolean immutable;
+ private Map data;
Review comment:
Missing generics.
##########
File path: src/main/java/org/apache/commons/collections4/MapBuilder.java
##########
@@ -0,0 +1,115 @@
+package org.apache.commons.collections4;
+
+import org.apache.commons.collections4.map.HashedMap;
+
+import java.util.*;
+
+/**
+ * Defines an Helper Builder that generates a {@code Map}
+ * A Builder class to help decide which type of map to use based on simple
requirements.
+ * Currently It takes care of only basic types of Maps and can be extended to
different types of Maps available in the ecosystem.
+ *
+ * <pre>{@code
+ * Map builderMap = new
MapBuilder().setIterationOrder(MapBuilder.KeyOrder.INSERTION_ORDER).build();
+ * builderMap.put("A", 1);
+ * builderMap.put("X", 24);
+ * builderMap.put("B", 2);
+ * builderMap.put("Y", 26);
+ * }</pre>
+ *
+ * @author Amita Pradhan
+ */
+public class MapBuilder {
+
+ private Comparator comparator;
Review comment:
Missing generics.
##########
File path: src/test/java/org/apache/commons/collections4/MapBuilderTest.java
##########
@@ -0,0 +1,122 @@
+package org.apache.commons.collections4;
+
+import org.junit.Assert;
+import org.junit.jupiter.api.Test;
+
+import java.util.Comparator;
+import java.util.HashMap;
+import java.util.Map;
+
+import static org.junit.jupiter.api.Assertions.*;
+
+/**
+ * Test Cases for Map Builder
+ */
+class MapBuilderTest {
Review comment:
IMO, you do not/should not need to test the behavior of Map
implementations. Checking the type of the built instance is enough.
##########
File path: src/main/java/org/apache/commons/collections4/MapBuilder.java
##########
@@ -0,0 +1,115 @@
+package org.apache.commons.collections4;
+
+import org.apache.commons.collections4.map.HashedMap;
+
+import java.util.*;
+
+/**
+ * Defines an Helper Builder that generates a {@code Map}
+ * A Builder class to help decide which type of map to use based on simple
requirements.
+ * Currently It takes care of only basic types of Maps and can be extended to
different types of Maps available in the ecosystem.
+ *
+ * <pre>{@code
+ * Map builderMap = new
MapBuilder().setIterationOrder(MapBuilder.KeyOrder.INSERTION_ORDER).build();
+ * builderMap.put("A", 1);
+ * builderMap.put("X", 24);
+ * builderMap.put("B", 2);
+ * builderMap.put("Y", 26);
+ * }</pre>
+ *
+ * @author Amita Pradhan
+ */
+public class MapBuilder {
+
+ private Comparator comparator;
+ private KeyOrder iterationOrder;
+ private boolean synchronizedMap;
+ private boolean immutable;
+ private Map data;
+
+ public MapBuilder() {
Review comment:
I only see this useful if you cover all Map implementations in the JDK.
It's really the only way IMO to see if this builder is handy or not but I am
somewhat doubtful in general. Let's see where this goes from here... For, me
it's simpler to use the right class in the right place. For example, how do I
build a `SortedMap<Foo, Bar>` and is returned to the call site as a
`SortedMap<Foo, Bar>`?
##########
File path: src/main/java/org/apache/commons/collections4/MapBuilder.java
##########
@@ -0,0 +1,115 @@
+package org.apache.commons.collections4;
+
+import org.apache.commons.collections4.map.HashedMap;
+
+import java.util.*;
+
+/**
+ * Defines an Helper Builder that generates a {@code Map}
+ * A Builder class to help decide which type of map to use based on simple
requirements.
+ * Currently It takes care of only basic types of Maps and can be extended to
different types of Maps available in the ecosystem.
+ *
+ * <pre>{@code
+ * Map builderMap = new
MapBuilder().setIterationOrder(MapBuilder.KeyOrder.INSERTION_ORDER).build();
+ * builderMap.put("A", 1);
+ * builderMap.put("X", 24);
+ * builderMap.put("B", 2);
+ * builderMap.put("Y", 26);
+ * }</pre>
+ *
+ * @author Amita Pradhan
+ */
+public class MapBuilder {
+
+ private Comparator comparator;
+ private KeyOrder iterationOrder;
+ private boolean synchronizedMap;
+ private boolean immutable;
+ private Map data;
+
+ public MapBuilder() {
+ comparator = null;
+ iterationOrder = KeyOrder.RANDOM;
+ synchronizedMap = false;
+ immutable = false;
+ data = null;
+ }
+
+ /*
+ Sets the comparator to be used to decide the Iteration order in case of
iterationOrder = COMPARATOR_ORDER;
+ */
+ public MapBuilder setComparator(Comparator comparator) {
+ this.comparator = comparator;
+ return this;
+ }
+
+ /*
+ Sets the Iteration order to be used from [RANDOM, NATURAL_ORDER,
INSERTION_ORDER, COMPARATOR_ORDER]
+ */
+ public MapBuilder setIterationOrder(KeyOrder iterationOrder) {
+ this.iterationOrder = iterationOrder;
+ return this;
+ }
+
+ /*
+ Since most of the maps are not inherently thread safe , this option
provides the option if the map has to be synchronised or not
+ */
+ public MapBuilder setSynchronizedMap(boolean synchronizedMap) {
+ this.synchronizedMap = synchronizedMap;
+ return this;
+ }
+
+ /*
+ Option to create a immutable map from the provided data
+ */
+ public MapBuilder setImmutable(boolean immutable) {
+ this.immutable = immutable;
+ return this;
+ }
+
+ /*
+ Populates the Map with some preexisting data. All the selected conditions
will be automatically applied to the existing data
+ */
+ public MapBuilder setData(Map data) {
+ this.data = data;
+ return this;
+ }
+
+ /*
+ Builder Method which takes care of all the conditions and returns the
required Map.
+ */
+ public Map build() {
+ Map map;
+ switch (iterationOrder) {
+ case NATURAL_ORDER :
+ case COMPARATOR_ORDER:
+ map = new TreeMap(comparator);
+ break;
+ case INSERTION_ORDER :
+ map = new LinkedHashMap();
+ break;
+ default:
+ map = new HashedMap();
+ break;
+ }
+
+ if(MapUtils.isNotEmpty(data)) {
+ map.putAll(data);
+ }
+
+ if(synchronizedMap) {
+ map = Collections.synchronizedMap(map);
+ }
+
+ if(immutable) {
+ map = Collections.unmodifiableMap(map);
+ }
+
+ return map;
+ }
+
+ enum KeyOrder
+ {
+ RANDOM, NATURAL_ORDER, INSERTION_ORDER, COMPARATOR_ORDER;
Review comment:
The term RANDOM is not correct since you are not randomizing anything.
##########
File path: src/test/java/org/apache/commons/collections4/MapBuilderTest.java
##########
@@ -0,0 +1,122 @@
+package org.apache.commons.collections4;
+
+import org.junit.Assert;
+import org.junit.jupiter.api.Test;
+
+import java.util.Comparator;
+import java.util.HashMap;
+import java.util.Map;
+
+import static org.junit.jupiter.api.Assertions.*;
+
+/**
+ * Test Cases for Map Builder
+ */
+class MapBuilderTest {
+
+ @Test
+ void setComparator() {
+ // Null Comparator
+ Map myMap = new HashMap();
Review comment:
Missing generics.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
[email protected]
