[
https://issues.apache.org/jira/browse/OPENNLP-1471?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=17690897#comment-17690897
]
ASF GitHub Bot commented on OPENNLP-1471:
-----------------------------------------
kinow commented on code in PR #505:
URL: https://github.com/apache/opennlp/pull/505#discussion_r1111234745
##########
opennlp-tools/src/main/java/opennlp/tools/dictionary/Dictionary.java:
##########
@@ -343,9 +343,41 @@ public boolean contains(Object obj) {
result = entrySet.contains(new StringListWrapper(new
StringList(str)));
}
-
return result;
}
+
+ @Override
+ public boolean equals(Object o) {
+ if (! (o instanceof Set)) {
+ return false;
+ } else {
Review Comment:
We can drop the `else` I think? Just a
```
if (! (o instanceof Set)) {
return False;
}
// rest of the code
```
##########
opennlp-tools/src/main/java/opennlp/tools/dictionary/Dictionary.java:
##########
@@ -343,9 +343,41 @@ public boolean contains(Object obj) {
result = entrySet.contains(new StringListWrapper(new
StringList(str)));
}
-
return result;
}
+
+ @Override
+ public boolean equals(Object o) {
+ if (! (o instanceof Set)) {
+ return false;
+ } else {
+ Set<String> toCheck = (Set<String>) o;
+ if (entrySet.size() != toCheck.size()) {
+ return false;
+ } else {
+ boolean isEqual = true; // will flip if one element is not equal
+ Iterator<StringListWrapper> entrySetIter = entrySet.iterator();
+ Iterator<String> toCheckIter = toCheck.iterator();
+ for (int i = 0; i < entrySet.size(); i++) {
+ StringListWrapper entry = entrySetIter.next();
Review Comment:
I think it would be faster to iterate on the entrySet,
```
for (StringListWrapper entry : entrySet) {
```
Then I think we also won't need the `Iterator<StringListWrapper>
entrySetIter = entrySet.iterator();` declaration.
##########
opennlp-tools/src/main/java/opennlp/tools/dictionary/Dictionary.java:
##########
@@ -343,9 +343,41 @@ public boolean contains(Object obj) {
result = entrySet.contains(new StringListWrapper(new
StringList(str)));
}
-
return result;
}
+
+ @Override
+ public boolean equals(Object o) {
+ if (! (o instanceof Set)) {
+ return false;
+ } else {
+ Set<String> toCheck = (Set<String>) o;
+ if (entrySet.size() != toCheck.size()) {
+ return false;
+ } else {
+ boolean isEqual = true; // will flip if one element is not equal
+ Iterator<StringListWrapper> entrySetIter = entrySet.iterator();
+ Iterator<String> toCheckIter = toCheck.iterator();
+ for (int i = 0; i < entrySet.size(); i++) {
+ StringListWrapper entry = entrySetIter.next();
+ if (isCaseSensitive) {
+ isEqual = entry.stringList.equals(new
StringList(toCheckIter.next()));
+ } else {
+ isEqual = entry.stringList.compareToIgnoreCase(new
StringList(toCheckIter.next()));
+ }
+ if (!isEqual) {
+ break;
+ }
+ }
+ return isEqual;
+ }
+ }
+ }
Review Comment:
I think this one has the suggestions above, and doesn't need the `isEqual`
variable.
```suggestion
@Override
public boolean equals(Object o) {
if (! (o instanceof Set)) {
return false;
}
Set<String> toCheck = (Set<String>) o;
if (entrySet.size() != toCheck.size()) {
return false;
}
Iterator<String> toCheckIter = toCheck.iterator();
for (StringListWrapper entry : entrySet) {
if (isCaseSensitive) {
if (!entry.stringList.equals(new
StringList(toCheckIter.next()))) {
return false;
}
} else {
if (!entry.stringList.compareToIgnoreCase(new
StringList(toCheckIter.next()))) {
return false;
}
}
}
return true;
}
```
##########
opennlp-tools/src/main/java/opennlp/tools/dictionary/Dictionary.java:
##########
@@ -343,9 +343,41 @@ public boolean contains(Object obj) {
result = entrySet.contains(new StringListWrapper(new
StringList(str)));
}
-
return result;
}
+
+ @Override
+ public boolean equals(Object o) {
+ if (! (o instanceof Set)) {
+ return false;
+ } else {
+ Set<String> toCheck = (Set<String>) o;
+ if (entrySet.size() != toCheck.size()) {
+ return false;
+ } else {
+ boolean isEqual = true; // will flip if one element is not equal
+ Iterator<StringListWrapper> entrySetIter = entrySet.iterator();
+ Iterator<String> toCheckIter = toCheck.iterator();
+ for (int i = 0; i < entrySet.size(); i++) {
+ StringListWrapper entry = entrySetIter.next();
+ if (isCaseSensitive) {
+ isEqual = entry.stringList.equals(new
StringList(toCheckIter.next()));
+ } else {
+ isEqual = entry.stringList.compareToIgnoreCase(new
StringList(toCheckIter.next()));
+ }
+ if (!isEqual) {
+ break;
+ }
Review Comment:
I think we can use `return` directly instead of `isEqual`?
##########
opennlp-tools/src/main/java/opennlp/tools/dictionary/Dictionary.java:
##########
@@ -343,9 +343,41 @@ public boolean contains(Object obj) {
result = entrySet.contains(new StringListWrapper(new
StringList(str)));
}
-
return result;
}
+
+ @Override
+ public boolean equals(Object o) {
+ if (! (o instanceof Set)) {
+ return false;
+ } else {
+ Set<String> toCheck = (Set<String>) o;
+ if (entrySet.size() != toCheck.size()) {
+ return false;
+ } else {
Review Comment:
Same here, `else` is optional`. Maybe having less "levels" makes the code
easier to read?
> Ensure Dictionary#asStringSet() implements hashCode() and equals() correctly
> ----------------------------------------------------------------------------
>
> Key: OPENNLP-1471
> URL: https://issues.apache.org/jira/browse/OPENNLP-1471
> Project: OpenNLP
> Issue Type: Improvement
> Affects Versions: 2.1.0, 2.1.1
> Reporter: Martin Wiesner
> Assignee: Martin Wiesner
> Priority: Minor
>
> The tests (a) {{DictionaryAsSetCaseInsensitiveTest}} and
> {{(b) DictionaryAsSetCaseSensitiveTest}} have an open TODO that points to a
> bug in Dictionary#asStringSet()
>
> {quote}// TODO: should it be equal??
> Assertions.assertNotSame(setA.hashCode(), setB.hashCode());
> {quote}
> To cure this, the implementation of Dictionary#asStringSet() needs override
> hashCode and equals properly.
> After fixing the implementation, the following assertion must hold:
> {quote}Assertions.assertEquals(setA, setB);
> Assertions.assertEquals(setA.hashCode(), setB.hashCode());
> {quote}
> where setA and setB are obtained via {{{}Dictionary#asStringSet(){}}}.
--
This message was sent by Atlassian Jira
(v8.20.10#820010)
