Martin Mucha has posted comments on this change. Change subject: core: wrapper of HashMap for counting number of objects ......................................................................
Patch Set 6: (5 comments) answers. http://gerrit.ovirt.org/#/c/26402/6/backend/manager/modules/bll/src/main/java/org/ovirt/engine/core/bll/network/ObjectCounter.java File backend/manager/modules/bll/src/main/java/org/ovirt/engine/core/bll/network/ObjectCounter.java: Line 5: Line 6: import org.ovirt.engine.core.utils.log.Log; Line 7: import org.ovirt.engine.core.utils.log.LogFactory; Line 8: Line 9: class ObjectCounter<T> { > Do you plan to add unit test? not now. I'm not planning usage of this class outside of this package if you're addressing visibility. Line 10: Line 11: private static final Log log = LogFactory.getLog(ObjectCounter.class); Line 12: Line 13: private Map<T, ModifiableInteger> map = new HashMap<>(); Line 25: return true; Line 26: } else if (allowDuplicate) { Line 27: counter.increment(); Line 28: return true; Line 29: } else { > It's not necessary to have the else block. there has to be return false. And constructs which 'returns' making rest of method not executed are correctly used as method invariants. (more generally is used by LOC metric fanciers, which I do not belong to). Line 30: return false; Line 31: } Line 32: } Line 33: Line 31: } Line 32: } Line 33: Line 34: Line 35: public boolean remove(T key) { > We don't use comments succeeding code, please change to be before the line Done Line 36: ModifiableInteger counter = map.get(key); Line 37: if (counter == null) { Line 38: //it's not there, ignore; Line 39: return false; //key was not there! So it was not removed. Line 49: map.remove(key); Line 50: return true; //key was removed. Line 51: } Line 52: Line 53: return false; //key is still present with lessened count. > Not sure what is false meaning here.. safety. Line 54: Line 55: } Line 56: Line 57: public boolean contains(T key) { Line 74: } Line 75: Line 76: public int increment() { Line 77: count++; Line 78: return count; > Why do you need the return value? not to have to use getter after incrementation. Nor to client code try to fiddle with numbers wisely incrementing one by its own. Seems normal to me. It's state modifying method returning new value. Take a look at ArrayList.add():boolean. It could also return void and let all callers call contains prior and after calling method add. Line 79: } Line 80: Line 81: public int decrement() { Line 82: count--; -- To view, visit http://gerrit.ovirt.org/26402 To unsubscribe, visit http://gerrit.ovirt.org/settings Gerrit-MessageType: comment Gerrit-Change-Id: I38b9ead37a8ebfc56103b87c65ba582a84f4dda6 Gerrit-PatchSet: 6 Gerrit-Project: ovirt-engine Gerrit-Branch: master Gerrit-Owner: Martin Mucha <[email protected]> Gerrit-Reviewer: Martin Mucha <[email protected]> Gerrit-Reviewer: Mike Kolesnik <[email protected]> Gerrit-Reviewer: Moti Asayag <[email protected]> Gerrit-Reviewer: Yevgeny Zaspitsky <[email protected]> Gerrit-Reviewer: [email protected] Gerrit-Reviewer: oVirt Jenkins CI Server Gerrit-HasComments: Yes _______________________________________________ Engine-patches mailing list [email protected] http://lists.ovirt.org/mailman/listinfo/engine-patches
