rikkarth commented on code in PR #412:
URL:
https://github.com/apache/commons-configuration/pull/412#discussion_r1589676819
##########
src/main/java/org/apache/commons/configuration2/AbstractConfiguration.java:
##########
@@ -419,6 +419,20 @@ public final boolean containsKey(final String key) {
}
}
+ /**
+ * {@inheritDoc} This implementation handles synchronization and delegates
to {@code containsKeyInternal()}.
+ */
+ @Override
+ public final boolean containsValue(final String value) {
+ Objects.requireNonNull(value, "Parameter \"value\" cannot be null");
Review Comment:
containsValue delegates to `containsKeyInternal()`.
`containsKeyInternal()` implementation can be different depending on the
implementing class.
If null is unavoidable we should always encourage safe null handling
practices, such as using Objects.requireNonNull or conditional checks, to
minimize the risk of (unexpected, unmanaged or uncontrolled) null pointer
exceptions.
While `new HashMap().containsValue(null)` returns false, this implementation
is inspired on `java.util.HashTable.contains()` and not HashMap which is also
`java.util.Properties` implementation (extends HashTable).
```java
// HashTable contains Implementation checks null at entry point
public synchronized boolean contains(Object value) {
if (value == null) {
throw new NullPointerException();
}
Entry<?,?> tab[] = table;
for (int i = tab.length ; i-- > 0 ;) {
for (Entry<?,?> e = tab[i] ; e != null ; e = e.next) {
if (e.value.equals(value)) {
return true;
}
}
}
return false;
}
```
I find this to be a much more coherent implementation since it doesn't fail
silently and explicitely warns the user that the input value is null.
The reason for the "custom" message is so we can control exactly how
containsValue fails.
What do you think?
--
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: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
