garydgregory commented on code in PR #412:
URL:
https://github.com/apache/commons-configuration/pull/412#discussion_r1589520428
##########
src/main/java/org/apache/commons/configuration2/AbstractConfiguration.java:
##########
@@ -1517,6 +1547,25 @@ protected int sizeInternal() {
return size;
}
+ /**
+ * Checks if the specified value exists in the properties structure mapped
by the provided keys.
+ *
+ * @param keys an Iterator of String keys to search for the value
+ * @param value the String value to search for in the properties
+ * @return true if the value is found in the properties, false otherwise
+ * @throws NullPointerException if keys or value is null
+ */
+ protected boolean contains(Iterator<String> keys, final String value) {
Review Comment:
Use `final` where you can, here, `keys` can be `final`.
##########
src/main/java/org/apache/commons/configuration2/ImmutableConfiguration.java:
##########
@@ -62,6 +62,20 @@ public interface ImmutableConfiguration {
*/
boolean containsKey(String key);
+ /**
+ * Returns true if this configuration contains one or more matches to this
value. This operation stops at first
Review Comment:
Be consistent in your Javadoc comments. In this case, "contains" methods
that return a boolean are "testing" for the presence of something, so you can
start the comment with "Tests" like "Tests whether this or that happens".
##########
src/test/java/org/apache/commons/configuration2/NonCloneableConfiguration.java:
##########
@@ -45,6 +45,11 @@ protected boolean containsKeyInternal(final String key) {
return false;
}
+ @Override
Review Comment:
Javadoc
##########
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:
- Replace API calls of this pattern with simply:
`Objects.requireNonNull(value, "value");`
See the rest of the code base.
- Why does this check need to happen here at all? After all I don't get an
NPE when I ask for a null value from a `HashMap`, for example ` new
HashMap().containsValue(null)` returns false.
##########
src/main/java/org/apache/commons/configuration2/ImmutableConfiguration.java:
##########
@@ -62,6 +62,20 @@ public interface ImmutableConfiguration {
*/
boolean containsKey(String key);
+ /**
+ * Returns true if this configuration contains one or more matches to this
value. This operation stops at first
+ * match but may be more expensive than the {@link #containsKey
containsKey} method.
+ *
+ * @param value value whose presence in this configuration is to be tested
+ * @return {@code true} if this configuration maps one or more keys to the
specified value
+ * @throws NullPointerException if the value is {@code null}
+ * @throws UnsupportedOperationException if this method is not supported
by the implementing class.
+ */
+ default boolean containsValue(String value) {
+ throw new UnsupportedOperationException(
+ String.format("Class %s does not support this method.",
this.getClass()));
Review Comment:
The message is redundant and bad, it read like "Class class com.example.Foo
does not...".
You must have not tested this case. A simpler message could just say
`getClass().getName() + ".myMethod"`
##########
src/main/java/org/apache/commons/configuration2/DatabaseConfiguration.java:
##########
@@ -478,6 +478,21 @@ protected Boolean performOperation() throws SQLException {
return result != null && result.booleanValue();
}
+ @Override
+ protected boolean containsValueInternal(final String value) {
+ final AbstractJdbcOperation<Boolean> op = new
AbstractJdbcOperation<Boolean>(ConfigurationErrorEvent.READ,
ConfigurationErrorEvent.READ, value, null) {
+ @Override
+ protected Boolean performOperation() throws SQLException {
+ try (ResultSet rs =
openResultSet(String.format(SQL_GET_PROPERTY, table, valueColumn), false,
value)) {
+ return rs.next();
+ }
+ }
+ };
+
Review Comment:
No need for extra whitespace (blank lines). If you want to call out a code
fragment, then you can use an inline `//` comment to alert the reader to
something specific.
##########
src/main/java/org/apache/commons/configuration2/ImmutableConfiguration.java:
##########
@@ -62,6 +62,20 @@ public interface ImmutableConfiguration {
*/
boolean containsKey(String key);
+ /**
+ * Returns true if this configuration contains one or more matches to this
value. This operation stops at first
+ * match but may be more expensive than the {@link #containsKey
containsKey} method.
+ *
+ * @param value value whose presence in this configuration is to be tested
+ * @return {@code true} if this configuration maps one or more keys to the
specified value
+ * @throws NullPointerException if the value is {@code null}
+ * @throws UnsupportedOperationException if this method is not supported
by the implementing class.
+ */
Review Comment:
Missing Javadoc since tag.
##########
src/test/java/org/apache/commons/configuration2/TestAbstractConfiguration.java:
##########
@@ -78,6 +78,49 @@ public void testAddPropertyDirect() {
assertEquals(expected, list);
}
+ @Test
+ void testContainsValue(){
+ Configuration config = getConfiguration();
+ assertTrue(config.containsValue("value1"), "should return true for
class " + this.getClass());
+ assertFalse(config.containsValue("value99999"), "should return false
for class " + this.getClass());
+ }
+
+ @Test
+ void testContains(){
+ AbstractConfiguration config = getConfiguration();
+
Review Comment:
No need to blank lines. See my previous comment.
##########
src/main/java/org/apache/commons/configuration2/AbstractConfiguration.java:
##########
@@ -429,6 +443,22 @@ public final boolean containsKey(final String key) {
*/
protected abstract boolean containsKeyInternal(String key);
+ /**
+ * Returns true if this configuration contains one or more matches to this
value. This operation stops at first
+ * match but may be more expensive than the {@link #containsKeyInternal
containsKey} method.
+ * <p>
+ * The implementation of this method will be different depending on the
type of Configuration used.
+ *
+ * <p>Note that this method is identical in functionality to
+ * {@link #containsValue containsValue}, (which is part of the {@link
ImmutableConfiguration} interface).
+ *
+ * @param value a value to search for
+ * @return {@code true} if and only if some key maps to the {@code value}
argument in this hashtable as determined
+ * by the {@code equals} method; {@code false} otherwise.
+ * @throws NullPointerException if the value is {@code null}
+ */
Review Comment:
New elements that are public or private should be documented with a Javadoc
`@since` tag.
##########
src/main/java/org/apache/commons/configuration2/AbstractConfiguration.java:
##########
@@ -1517,6 +1547,25 @@ protected int sizeInternal() {
return size;
}
+ /**
+ * Checks if the specified value exists in the properties structure mapped
by the provided keys.
+ *
+ * @param keys an Iterator of String keys to search for the value
+ * @param value the String value to search for in the properties
+ * @return true if the value is found in the properties, false otherwise
+ * @throws NullPointerException if keys or value is null
+ */
+ protected boolean contains(Iterator<String> keys, final String value) {
+ Objects.requireNonNull(keys, "Parameter \"keys\" cannot be null");
Review Comment:
See my previous comment RE `Objects.requireNonNull()`.
Same comment RE null checking in the first place. Why not return false?
##########
src/test/java/org/apache/commons/configuration2/TestAbstractConfiguration.java:
##########
@@ -78,6 +78,49 @@ public void testAddPropertyDirect() {
assertEquals(expected, list);
}
+ @Test
+ void testContainsValue(){
+ Configuration config = getConfiguration();
Review Comment:
Use `final`.
##########
src/main/java/org/apache/commons/configuration2/ImmutableConfiguration.java:
##########
@@ -62,6 +62,20 @@ public interface ImmutableConfiguration {
*/
boolean containsKey(String key);
+ /**
+ * Returns true if this configuration contains one or more matches to this
value. This operation stops at first
+ * match but may be more expensive than the {@link #containsKey
containsKey} method.
+ *
+ * @param value value whose presence in this configuration is to be tested
+ * @return {@code true} if this configuration maps one or more keys to the
specified value
+ * @throws NullPointerException if the value is {@code null}
+ * @throws UnsupportedOperationException if this method is not supported
by the implementing class.
+ */
+ default boolean containsValue(String value) {
Review Comment:
Can this method be implemented in terms of other interface methods instead
of throwing an exception?
##########
src/test/java/org/apache/commons/configuration2/TestAbstractConfiguration.java:
##########
@@ -78,6 +78,49 @@ public void testAddPropertyDirect() {
assertEquals(expected, list);
}
+ @Test
+ void testContainsValue(){
Review Comment:
The style of the component is to use `public` test methods prefixed with
"test" in the name and camel-case. Look at other tests for examples.
--
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]
