Claudenw commented on code in PR #412:
URL:
https://github.com/apache/commons-configuration/pull/412#discussion_r1586333505
##########
src/main/java/org/apache/commons/configuration2/JNDIConfiguration.java:
##########
@@ -147,6 +147,26 @@ protected boolean containsKeyInternal(String key) {
}
}
+ @Override
+ protected boolean containsValueInternal(final String value) {
+ return contains(getKeys(), value);
+ }
+
+ private boolean contains(Iterator<String> keys, final String value) {
+ if (keys.hasNext()) {
+ String nextKey = keys.next();
+ Object valueFromKey = getProperty(nextKey);
+
+ if (valueFromKey.equals(value)) {
+ return true;
+ }
+
+ contains(keys, value);
+ }
+
+ return false;
Review Comment:
See comments for similar method above.
##########
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), true,
value)) {
+ return rs.next();
Review Comment:
If I read the SQL_GET_PROPERTY correctly as well as the what you are passing
in as arguments, you have used up all the prepared statement parameters and the
`true` value in this line should be `false`.
**WARNING** I may be mistaken.
##########
src/main/java/org/apache/commons/configuration2/web/BaseWebConfiguration.java:
##########
@@ -68,6 +69,26 @@ protected boolean containsKeyInternal(final String key) {
return getPropertyInternal(key) != null;
}
+ @Override
+ protected boolean containsValueInternal(final String value) {
+ return contains(getKeys(), value);
+ }
+
+ private boolean contains(Iterator<String> keys, final String value) {
+ if (keys.hasNext()) {
+ String nextKey = keys.next();
+ Object valueFromKey = getProperty(nextKey);
+
+ if (valueFromKey.equals(value)) {
+ return true;
+ }
+
+ contains(keys, value);
+ }
+
+ return false;
+ }
Review Comment:
See comments for similar method above.
Do all of these classes implement a common interface or class where a common
method might be placed?
##########
src/main/java/org/apache/commons/configuration2/AbstractHierarchicalConfiguration.java:
##########
@@ -455,6 +455,26 @@ protected boolean containsKeyInternal(final String key) {
return getPropertyInternal(key) != null;
}
+ @Override
+ protected boolean containsValueInternal(final String value) {
+ return contains(getKeys(), value);
+ }
+
+ private boolean contains(Iterator<String> keys, final String value) {
+ if (keys.hasNext()) {
+ String nextKey = keys.next();
+ Object valueFromKey = getProperty(nextKey);
+
+ if (valueFromKey.equals(value)) {
+ return true;
Review Comment:
`valueFromKey` can be null here. But `value` can not so reverse the
equality check `value.equals(valueFromKey)` then you can simplify further and
just use `value.equals(getProperty(keys.next()))`
##########
src/main/java/org/apache/commons/configuration2/AbstractHierarchicalConfiguration.java:
##########
@@ -455,6 +455,26 @@ protected boolean containsKeyInternal(final String key) {
return getPropertyInternal(key) != null;
}
+ @Override
+ protected boolean containsValueInternal(final String value) {
+ return contains(getKeys(), value);
+ }
+
+ private boolean contains(Iterator<String> keys, final String value) {
+ if (keys.hasNext()) {
+ String nextKey = keys.next();
+ Object valueFromKey = getProperty(nextKey);
+
+ if (valueFromKey.equals(value)) {
+ return true;
+ }
+
+ contains(keys, value);
+ }
+
+ return false;
+ }
+
Review Comment:
I don't like this recursion for no reason.
If you use a `Supplier<String>` you can abstract this out and make a static
method that is callable from the other location where you have what looks like
a cut and past resuse of this method.
For this method you could simply do:
```
while (keys.hasNext()) {
if (value.equals(getProperty(keys.next())) {
return true;
}
}
return false
```
--
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]
