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?
--
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]
