Alona Kaplan has posted comments on this change. Change subject: core: Introduce NetworkAttachment entity ......................................................................
Patch Set 19: (2 comments) https://gerrit.ovirt.org/#/c/32411/19/backend/manager/modules/common/src/main/java/org/ovirt/engine/core/common/businessentities/network/NetworkAttachment.java File backend/manager/modules/common/src/main/java/org/ovirt/engine/core/common/businessentities/network/NetworkAttachment.java: Line 22: private Guid networkId; Line 23: Line 24: private Guid nicId; Line 25: Line 26: // TODO: Add validation for the name for nic or bond > note: all mine TODOs has format: "//TODO MM: .*" Others are not mine todos. So it seems we agree this TODO is redundant, please remove it (if you haven't already done it in the following patches). Line 27: private String nicName; Line 28: Line 29: private IpConfiguration ipConfiguration; Line 30: private Map<String, String> properties; Line 107: return false; Line 108: if (getClass() != obj.getClass()) Line 109: return false; Line 110: NetworkAttachment other = (NetworkAttachment) obj; Line 111: if (id == null) { > 'equals' and hashcode are automatically generated methods by IDE. Please te I'm not generating the equals method. IMO, it is more readable to use Objects.equals(..) instead of all this null checks. Using just the key properties in equals and hashCode gives us the option to store the entity properly in a HashSet. I saw a lot of bugs around this area (for example- the entity is stored in a HashSet=> one of its properties is changed=> hashSet.contains(entity) returns false). I understand that changing the equals and hashCode behavior can cause bugs, but if you can, please verify the current use cases of the methods doesn't need all the properties to be compared. If it's very difficult to do this verification I won't insist on changing the methods. Line 112: if (other.id != null) Line 113: return false; Line 114: } else if (!id.equals(other.id)) Line 115: return false; -- To view, visit https://gerrit.ovirt.org/32411 To unsubscribe, visit https://gerrit.ovirt.org/settings Gerrit-MessageType: comment Gerrit-Change-Id: Ided81dc2be68dc4c7a9d491697f887cdae477a2c Gerrit-PatchSet: 19 Gerrit-Project: ovirt-engine Gerrit-Branch: master Gerrit-Owner: Moti Asayag <[email protected]> Gerrit-Reviewer: Alona Kaplan <[email protected]> Gerrit-Reviewer: Jenkins CI Gerrit-Reviewer: Martin Mucha <[email protected]> Gerrit-Reviewer: Moti Asayag <[email protected]> Gerrit-Reviewer: [email protected] Gerrit-HasComments: Yes _______________________________________________ Engine-patches mailing list [email protected] http://lists.ovirt.org/mailman/listinfo/engine-patches
