Copilot commented on code in PR #15633:
URL: https://github.com/apache/dubbo/pull/15633#discussion_r2280119367
##########
dubbo-registry/dubbo-registry-api/src/main/java/org/apache/dubbo/registry/client/DefaultServiceInstance.java:
##########
@@ -305,28 +315,61 @@ public boolean equals(Object o) {
return false;
}
DefaultServiceInstance that = (DefaultServiceInstance) o;
- boolean equals = Objects.equals(getServiceName(),
that.getServiceName())
- && Objects.equals(getHost(), that.getHost())
- && Objects.equals(getPort(), that.getPort());
- for (Map.Entry<String, String> entry : this.getMetadata().entrySet()) {
- if
(entry.getKey().equals(EXPORTED_SERVICES_REVISION_PROPERTY_NAME)) {
+ if (!Objects.equals(getServiceName(), that.getServiceName())
+ || !Objects.equals(getHost(), that.getHost())
+ || !Objects.equals(getPort(), that.getPort())) {
+ return false;
+ }
+
+ // compare map entry values by iterating over two metadata maps
(sorted) in a single pass.
+ Iterator<Map.Entry<String, String>> iterThis =
+ this.getMetadata().entrySet().iterator();
+ Iterator<Map.Entry<String, String>> iterThat =
+ that.getMetadata().entrySet().iterator();
+
+ boolean hasNext = iterThis.hasNext();
+ if (hasNext != iterThat.hasNext()) {
+ // keep consistency with hashCode, as any extra or less key would
result in a different hashCode.
+ return false;
+ }
+
+ while (hasNext) {
+ Map.Entry<String, String> entryThis = iterThis.next();
+ Map.Entry<String, String> entryThat = iterThat.next();
+
+ String key = entryThis.getKey();
+ if (!key.equals(entryThat.getKey())) {
+ // keep consistency with hashCode, as any extra or less key
would result in a different hashCode.
+ return false;
+ }
+
+ if (key.equals(EXPORTED_SERVICES_REVISION_PROPERTY_NAME) ||
key.equals(TIMESTAMP_KEY)) {
+ // skip entry value comparison.
continue;
}
- if (entry.getValue() == null) {
- equals = equals && (entry.getValue() ==
that.getMetadata().get(entry.getKey()));
- } else {
- equals = equals &&
entry.getValue().equals(that.getMetadata().get(entry.getKey()));
+
+ if (!Objects.equals(entryThis.getValue(), entryThat.getValue())) {
+ return false;
+ }
+
+ hasNext = iterThis.hasNext();
+ if (hasNext != iterThat.hasNext()) {
+ // keep consistency with hashCode, as any extra or less key
would result in a different hashCode.
+ return false;
}
}
- return equals;
+ return true;
Review Comment:
The equals method has become significantly more complex with manual iterator
management. The logic for checking iterator consistency is repeated multiple
times (lines 331, 355-358), making the code harder to maintain and understand.
##########
dubbo-registry/dubbo-registry-api/src/main/java/org/apache/dubbo/registry/client/DefaultServiceInstance.java:
##########
@@ -305,28 +315,61 @@ public boolean equals(Object o) {
return false;
}
DefaultServiceInstance that = (DefaultServiceInstance) o;
- boolean equals = Objects.equals(getServiceName(),
that.getServiceName())
- && Objects.equals(getHost(), that.getHost())
- && Objects.equals(getPort(), that.getPort());
- for (Map.Entry<String, String> entry : this.getMetadata().entrySet()) {
- if
(entry.getKey().equals(EXPORTED_SERVICES_REVISION_PROPERTY_NAME)) {
+ if (!Objects.equals(getServiceName(), that.getServiceName())
+ || !Objects.equals(getHost(), that.getHost())
+ || !Objects.equals(getPort(), that.getPort())) {
+ return false;
+ }
+
+ // compare map entry values by iterating over two metadata maps
(sorted) in a single pass.
+ Iterator<Map.Entry<String, String>> iterThis =
+ this.getMetadata().entrySet().iterator();
+ Iterator<Map.Entry<String, String>> iterThat =
+ that.getMetadata().entrySet().iterator();
+
+ boolean hasNext = iterThis.hasNext();
+ if (hasNext != iterThat.hasNext()) {
+ // keep consistency with hashCode, as any extra or less key would
result in a different hashCode.
+ return false;
+ }
+
+ while (hasNext) {
+ Map.Entry<String, String> entryThis = iterThis.next();
+ Map.Entry<String, String> entryThat = iterThat.next();
+
+ String key = entryThis.getKey();
+ if (!key.equals(entryThat.getKey())) {
+ // keep consistency with hashCode, as any extra or less key
would result in a different hashCode.
+ return false;
+ }
+
+ if (key.equals(EXPORTED_SERVICES_REVISION_PROPERTY_NAME) ||
key.equals(TIMESTAMP_KEY)) {
+ // skip entry value comparison.
continue;
}
- if (entry.getValue() == null) {
- equals = equals && (entry.getValue() ==
that.getMetadata().get(entry.getKey()));
- } else {
- equals = equals &&
entry.getValue().equals(that.getMetadata().get(entry.getKey()));
+
+ if (!Objects.equals(entryThis.getValue(), entryThat.getValue())) {
+ return false;
+ }
+
+ hasNext = iterThis.hasNext();
+ if (hasNext != iterThat.hasNext()) {
+ // keep consistency with hashCode, as any extra or less key
would result in a different hashCode.
+ return false;
}
}
- return equals;
+ return true;
Review Comment:
The equals method creates iterators and performs complex iteration logic
when a simple map comparison would be more efficient. Consider using
Map.equals() after filtering out the excluded keys
(EXPORTED_SERVICES_REVISION_PROPERTY_NAME and TIMESTAMP_KEY) into temporary
maps.
--
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]
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
