Martin Mucha has posted comments on this change. Change subject: core: wrapper of HashMap for counting number of objects ......................................................................
Patch Set 6: (6 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> { > No, I'm about having ObjectCounterTest class blackbox test added. 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 { > Not sure what you mean by your explanation.. ok, so maybe I did not understand you. If you're talking about necessity of using else block, because return in first then clause will do, so that ifs need not to be nested, then reply is: You're right. For compiler both notations are the same. But there's semantic difference for the reader. I like and use both notations where appropriate. This is highly subjective, when to use what, and pointless. Both notations are valid and adheres to oracles code conventions. In this situation I think the one currently used is better. If you mean something different, I cannot see it. Return boolean value for add method *is* needed by client code Line 30: return false; Line 31: } Line 32: } Line 33: 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. > Why is return value even necessary? Is it used? Doesn't seem like it.. to be consistent with add. Add method returns boolean whether item was added (used with different duplicates allowed settings). So reverse function should be the same. (inconsistent code is actually harder to maintain, definitely worse than 'not needed return value') but DONE. I'm dropping that. Line 54: Line 55: } Line 56: Line 57: public boolean contains(T key) { Line 57: public boolean contains(T key) { Line 58: return map.containsKey(key); Line 59: } Line 60: Line 61: private static class ModifiableInteger { > IMHO Counter is a better name Done Line 62: private int count; Line 63: Line 64: public ModifiableInteger(int initialValue) { Line 65: setCount(initialValue); Line 60: Line 61: private static class ModifiableInteger { Line 62: private int count; Line 63: Line 64: public ModifiableInteger(int initialValue) { > Yes. As the class usage is very limited and the only way it called is with Seems like nonsense to me, to have such default, because from client code perspective one will have to look what default constructor will choose for default value. And moreover client code want HIS value to be set, not different class chosen default. This is (by proper OO design) his responsibility to supply value HE want; Theoretically inner 'Counter' class (formerly ModifiableInteger) can change and then ObjectCounter functionality would be compromised. but DONE. Line 65: setCount(initialValue); Line 66: } Line 67: Line 68: public int getCount() { Line 74: } Line 75: Line 76: public int increment() { Line 77: count++; Line 78: return count; > You don't use the return value, so I ask again - why do you need it? still the same story. It's all about degree of naivity of naive solution(first released). Like in TDD — strictly you should write test, then empty method, and run that test prooving that empty method will not do. But nobody actually does that. So some 'naive' method body is used, which usually means '2 minutes to write'. You want super-naive solution, I, by default, opting for less naive. DONE. No return value. 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 <mmu...@redhat.com> Gerrit-Reviewer: Martin Mucha <mmu...@redhat.com> Gerrit-Reviewer: Mike Kolesnik <mkole...@redhat.com> Gerrit-Reviewer: Moti Asayag <masa...@redhat.com> Gerrit-Reviewer: Yevgeny Zaspitsky <yzasp...@redhat.com> Gerrit-Reviewer: automat...@ovirt.org 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
