Alon Bar-Lev has posted comments on this change.
Change subject: engine: Add infrastructure code for removal of parameter classes
......................................................................
Patch Set 8:
(11 comments)
....................................................
File
backend/manager/modules/common/src/main/java/org/ovirt/engine/core/common/utils/VdcParameter.java
Line 37: }
Line 38: VdcParameter otherParam = (VdcParameter) other;
Line 39: return getName().equals(otherParam.getName())
Line 40: && getJavaType().equals(otherParam.getJavaType())
Line 41: && getGuid().equals(otherParam.getGuid());
compare only guid, no?
Line 42: }
Line 43:
Line 44: @Override
Line 45: public int hashCode() {
Line 45: public int hashCode() {
Line 46: int hash = 7;
Line 47: hash = 31 * hash + (this.name != null ? this.name.hashCode() :
0);
Line 48: hash = 31 * hash + (this.javaType != null ?
this.javaType.hashCode() : 0);
Line 49: hash = 31 * hash + (this.guid != null ? this.guid.hashCode() :
0);
should be based on guid only
Line 50: return hash;
Line 51: }
Line 52:
Line 53: @Override
Line 57:
Line 58: public static VdcParameter create(String name, Class javaType,
String guidStr) {
Line 59: Guid guid = new Guid(guidStr);
Line 60: if (parametersMap.containsKey(guid)) {
Line 61: throw new InvalidParameterGuidException(name, guidStr,
parametersMap.get(guid));
you do not need guidStr as you have this at the VdcParameter on the right
Line 62: }
Line 63: parametersMap.put(guid, new VdcParameter(name, javaType,
guid));
Line 64: return parametersMap.get(guid);
Line 65: }
Line 59: Guid guid = new Guid(guidStr);
Line 60: if (parametersMap.containsKey(guid)) {
Line 61: throw new InvalidParameterGuidException(name, guidStr,
parametersMap.get(guid));
Line 62: }
Line 63: parametersMap.put(guid, new VdcParameter(name, javaType,
guid));
lock before put? so no problem in threads?
Line 64: return parametersMap.get(guid);
Line 65: }
Line 66:
Line 67: /**
Line 66:
Line 67: /**
Line 68: * Used by Json deserializer to retrieve the existing VdcParameter
Line 69: */
Line 70: public static VdcParameter getExisting(String guidStr) {
getByGuid() ?
Line 71: return parametersMap.get(new Guid(guidStr));
Line 72: }
....................................................
File
backend/manager/modules/common/src/main/java/org/ovirt/engine/core/common/utils/VdcParametersMap.java
Line 44: }
Line 45:
Line 46: public int size() {
Line 47: return paramsMap.size();
Line 48: }
I would have also added keys() or similar to get all entries in map to allow
iterations.
Line 49:
Line 50: @Override
Line 51: public boolean equals(Object obj) {
Line 52: if (this == obj) {
Line 62: if (size() != otherParamsMap.size()) {
Line 63: return false;
Line 64: }
Line 65: for (Entry<VdcParameter, Object> entry : paramsMap.entrySet())
{
Line 66: Object otherValue = otherParamsMap.get(entry.getKey(),
entry.getKey().getJavaType());
why don't you use the other class members directly? why go via the public
interface?
I also think I asked several times, why AbstractMap::equals is not enough for
compare?
Line 67: if (otherValue == null) {
Line 68: return false;
Line 69: }
Line 70: Object thisValue = entry.getValue();
Line 78: @Override
Line 79: public int hashCode() {
Line 80: int hash = 7;
Line 81: final int prime = 31;
Line 82: for (Entry<VdcParameter, Object> entry : paramsMap.entrySet())
{
why AbstractMap::hashCode is not enough?
Line 83: hash = prime * hash + entry.getKey().hashCode();
Line 84: hash = prime * hash + entry.getValue().hashCode();
Line 85: }
Line 86: return hash;
Line 95: for (Entry<VdcParameter, Object> entry : paramsMap.entrySet())
{
Line 96: copy.put(entry.getKey(), entry.getValue());
Line 97: }
Line 98: return copy;
Line 99: }
why not support Clonable interface and call clone on our map?
Line 100:
Line 101: /**
Line 102: * Used by Json deserializer. Copy the contents of the map by
calling
Line 103: * put method which does the type checking
Line 102: * Used by Json deserializer. Copy the contents of the map by
calling
Line 103: * put method which does the type checking
Line 104: */
Line 105: public void setParamsMap(Map<VdcParameter, Object> paramsMap) {
Line 106: this.paramsMap = new HashMap<VdcParameter, Object>();
same here, why not use clone?
Line 107: for (Entry<VdcParameter, Object> entry :
paramsMap.entrySet()) {
Line 108: put(entry.getKey(), entry.getValue());
Line 109: }
Line 110: }
....................................................
File
backend/manager/modules/utils/src/main/java/org/ovirt/engine/core/utils/serialization/json/JsonObjectDeserializer.java
Line 101: private static class VdcParameterDeserializer extends
KeyDeserializer {
Line 102: @Override
Line 103: public VdcParameter deserializeKey(String key,
DeserializationContext ctxt) throws IOException {
Line 104: String[] tokens = key.split(";");
Line 105: return VdcParameter.getExisting(tokens[2]);
not sure what is token[2], can you please explain?
Line 106: }
Line 107: }
--
To view, visit http://gerrit.ovirt.org/20414
To unsubscribe, visit http://gerrit.ovirt.org/settings
Gerrit-MessageType: comment
Gerrit-Change-Id: I6a0970e492c0eff561888a46b02e47645ff68fc3
Gerrit-PatchSet: 8
Gerrit-Project: ovirt-engine
Gerrit-Branch: master
Gerrit-Owner: Ravi Nori <rn...@redhat.com>
Gerrit-Reviewer: Alon Bar-Lev <alo...@redhat.com>
Gerrit-Reviewer: Barak Azulay <bazu...@redhat.com>
Gerrit-Reviewer: Liran Zelkha <lzel...@redhat.com>
Gerrit-Reviewer: Ravi Nori <rn...@redhat.com>
Gerrit-Reviewer: Yaniv Bronhaim <ybron...@redhat.com>
Gerrit-Reviewer: mooli tayer <mta...@redhat.com>
Gerrit-Reviewer: oVirt Jenkins CI Server
Gerrit-HasComments: Yes
_______________________________________________
Engine-patches mailing list
Engine-patches@ovirt.org
http://lists.ovirt.org/mailman/listinfo/engine-patches
