dsmiley commented on code in PR #3048:
URL: https://github.com/apache/solr/pull/3048#discussion_r1922851418
##########
solr/solrj/src/java/org/apache/solr/common/util/SimpleOrderedMap.java:
##########
@@ -86,6 +91,79 @@ public static SimpleOrderedMap<Object> of() {
* @return SimpleOrderedMap containing one key-value pair
*/
public static <T> SimpleOrderedMap<T> of(String name, T val) {
- return new SimpleOrderedMap<>(List.of(name, val));
+ return new SimpleOrderedMap<T>(List.of(name, val));
+ }
+
+ @Override
+ public boolean isEmpty() {
+ return nvPairs.isEmpty();
+ }
+
+ @Override
+ public boolean containsKey(final Object key) {
+ return super.get((String) key) != null;
+ }
+
+ @Override
+ public boolean containsValue(final Object value) {
+ int sz = size();
+ for (int i = 0; i < sz; i++) {
+ T val = getVal(i);
+ if (Objects.equals(value, val)) {
+ return true;
+ }
+ }
+ return false;
+ }
+
+ @Override
+ public T get(final Object key) {
+ return super.get((String) key);
+ }
+
+ @Override
+ public T put(final String key, final T value) {
+ T oldValue = get(key);
+ add(key, value);
+ return oldValue;
+ }
+
+ @Override
+ public T remove(final Object key) {
+ return super.remove((String) key);
+ }
+
+ @Override
+ public void putAll(final Map<? extends String, ? extends T> m) {
+ m.forEach(this::put);
+ }
+
+ @Override
+ public Set<String> keySet() {
+ var keys = new HashSet<String>();
+ for (int i = 0; i < nvPairs.size(); i += 2) {
+ keys.add((String) nvPairs.get(i));
+ }
+ return keys;
+ }
+
+ @Override
+ @SuppressWarnings({"unchecked"})
+ public Collection<T> values() {
Review Comment:
Please don't create new datastructures when calling methods you add
##########
solr/solrj/src/java/org/apache/solr/common/util/SimpleOrderedMap.java:
##########
@@ -86,6 +91,79 @@ public static SimpleOrderedMap<Object> of() {
* @return SimpleOrderedMap containing one key-value pair
*/
public static <T> SimpleOrderedMap<T> of(String name, T val) {
- return new SimpleOrderedMap<>(List.of(name, val));
+ return new SimpleOrderedMap<T>(List.of(name, val));
+ }
+
+ @Override
+ public boolean isEmpty() {
+ return nvPairs.isEmpty();
+ }
+
+ @Override
+ public boolean containsKey(final Object key) {
+ return super.get((String) key) != null;
+ }
+
+ @Override
+ public boolean containsValue(final Object value) {
+ int sz = size();
+ for (int i = 0; i < sz; i++) {
+ T val = getVal(i);
+ if (Objects.equals(value, val)) {
+ return true;
+ }
+ }
+ return false;
+ }
+
+ @Override
+ public T get(final Object key) {
+ return super.get((String) key);
+ }
+
+ @Override
+ public T put(final String key, final T value) {
+ T oldValue = get(key);
+ add(key, value);
+ return oldValue;
+ }
+
+ @Override
+ public T remove(final Object key) {
+ return super.remove((String) key);
+ }
+
+ @Override
+ public void putAll(final Map<? extends String, ? extends T> m) {
+ m.forEach(this::put);
+ }
+
+ @Override
+ public Set<String> keySet() {
+ var keys = new HashSet<String>();
Review Comment:
Please don't create new datastructures when calling methods you add
##########
solr/solrj/src/java/org/apache/solr/common/util/SimpleOrderedMap.java:
##########
@@ -86,6 +91,79 @@ public static SimpleOrderedMap<Object> of() {
* @return SimpleOrderedMap containing one key-value pair
*/
public static <T> SimpleOrderedMap<T> of(String name, T val) {
- return new SimpleOrderedMap<>(List.of(name, val));
+ return new SimpleOrderedMap<T>(List.of(name, val));
+ }
+
+ @Override
+ public boolean isEmpty() {
+ return nvPairs.isEmpty();
+ }
+
+ @Override
+ public boolean containsKey(final Object key) {
+ return super.get((String) key) != null;
Review Comment:
the value can actually be null in a NamedList, so this is inaccurate. Use
indexOf
##########
solr/solrj/src/java/org/apache/solr/common/util/SimpleOrderedMap.java:
##########
@@ -86,6 +91,79 @@ public static SimpleOrderedMap<Object> of() {
* @return SimpleOrderedMap containing one key-value pair
*/
public static <T> SimpleOrderedMap<T> of(String name, T val) {
- return new SimpleOrderedMap<>(List.of(name, val));
+ return new SimpleOrderedMap<T>(List.of(name, val));
+ }
+
+ @Override
+ public boolean isEmpty() {
+ return nvPairs.isEmpty();
+ }
+
+ @Override
+ public boolean containsKey(final Object key) {
+ return super.get((String) key) != null;
+ }
+
+ @Override
+ public boolean containsValue(final Object value) {
+ int sz = size();
+ for (int i = 0; i < sz; i++) {
+ T val = getVal(i);
+ if (Objects.equals(value, val)) {
+ return true;
+ }
+ }
+ return false;
+ }
+
+ @Override
+ public T get(final Object key) {
+ return super.get((String) key);
+ }
+
+ @Override
+ public T put(final String key, final T value) {
+ T oldValue = get(key);
+ add(key, value);
Review Comment:
add will not replace the existing value. This is why that method is called
"add" (vs put or set). I pointed you at NamedList asShallowMap which dealt
with this. This concern also led me to recommend deferring mutability.
##########
solr/solrj/src/test/org/apache/solr/common/util/SimpleOrderedMapTest.java:
##########
@@ -0,0 +1,125 @@
+package org.apache.solr.common.util;
+
+import java.util.Collection;
+import java.util.Map;
+import java.util.Set;
+import org.junit.Assert;
+import org.junit.Test;
+
+public class SimpleOrderedMapTest extends Assert {
Review Comment:
No; in Solr we extend SolrTestCase at a minimum for a unit test
##########
solr/solrj/src/java/org/apache/solr/common/util/SimpleOrderedMap.java:
##########
@@ -86,6 +91,79 @@ public static SimpleOrderedMap<Object> of() {
* @return SimpleOrderedMap containing one key-value pair
*/
public static <T> SimpleOrderedMap<T> of(String name, T val) {
- return new SimpleOrderedMap<>(List.of(name, val));
+ return new SimpleOrderedMap<T>(List.of(name, val));
+ }
+
Review Comment:
minor: (organizational) better to place these instance methods above the
static methods that you recently added
##########
solr/solrj/src/java/org/apache/solr/common/util/SimpleOrderedMap.java:
##########
@@ -86,6 +91,79 @@ public static SimpleOrderedMap<Object> of() {
* @return SimpleOrderedMap containing one key-value pair
*/
public static <T> SimpleOrderedMap<T> of(String name, T val) {
- return new SimpleOrderedMap<>(List.of(name, val));
+ return new SimpleOrderedMap<T>(List.of(name, val));
+ }
+
+ @Override
+ public boolean isEmpty() {
+ return nvPairs.isEmpty();
+ }
+
+ @Override
+ public boolean containsKey(final Object key) {
+ return super.get((String) key) != null;
+ }
+
+ @Override
+ public boolean containsValue(final Object value) {
+ int sz = size();
+ for (int i = 0; i < sz; i++) {
+ T val = getVal(i);
+ if (Objects.equals(value, val)) {
+ return true;
+ }
+ }
+ return false;
+ }
+
+ @Override
+ public T get(final Object key) {
+ return super.get((String) key);
+ }
+
+ @Override
+ public T put(final String key, final T value) {
+ T oldValue = get(key);
+ add(key, value);
+ return oldValue;
+ }
+
+ @Override
+ public T remove(final Object key) {
+ return super.remove((String) key);
+ }
+
+ @Override
+ public void putAll(final Map<? extends String, ? extends T> m) {
+ m.forEach(this::put);
+ }
+
+ @Override
+ public Set<String> keySet() {
+ var keys = new HashSet<String>();
+ for (int i = 0; i < nvPairs.size(); i += 2) {
+ keys.add((String) nvPairs.get(i));
+ }
+ return keys;
+ }
+
+ @Override
+ @SuppressWarnings({"unchecked"})
+ public Collection<T> values() {
+ var values = new ArrayList<T>();
+ for (int i = 1; i < nvPairs.size(); i += 2) {
+ values.add((T) nvPairs.get(i));
+ }
+ return values;
+ }
+
+ @Override
+ @SuppressWarnings({"unchecked"})
+ public Set<Entry<String, T>> entrySet() {
Review Comment:
Please don't create new datastructures when calling methods you add
##########
solr/solrj/src/test/org/apache/solr/common/util/SimpleOrderedMapTest.java:
##########
@@ -0,0 +1,125 @@
+package org.apache.solr.common.util;
+
+import java.util.Collection;
+import java.util.Map;
+import java.util.Set;
+import org.junit.Assert;
+import org.junit.Test;
+
+public class SimpleOrderedMapTest extends Assert {
+
+ private final SimpleOrderedMap<Integer> map = new SimpleOrderedMap<>();
Review Comment:
why have a field for the map instead of each test merely creating the maps
it needs? A field means need a lifecycle like `@After` to clear.
--
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.
To unsubscribe, e-mail: issues-unsubscr...@solr.apache.org
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscr...@solr.apache.org
For additional commands, e-mail: issues-h...@solr.apache.org
