mooli tayer has posted comments on this change.
Change subject: tools: Remove cumbersome NotificationMethod abstraction.
......................................................................
Patch Set 12:
(1 comment)
....................................................
File
backend/manager/modules/common/src/main/java/org/ovirt/engine/core/common/EventNotificationMethod.java
Line 6: public enum EventNotificationMethod {
Line 7: EMAIL(0);
Line 8:
Line 9: private int methodId;
Line 10: private static Map<Integer, EventNotificationMethod> mappings;
1.) My problem with it is the DRY principle. with switches we keep the value of
each enum value twice: once in the value field and again in the forMethodId
function - such solutions can cause problems in the future. More then that it's
non standard (though i'm sure that's debatable...)
2.) Holding the method name in the db is acceptable by me. It's a tradeoff
between storage space on one hand to the db being more readable and the mapping
being simpler on the other hand. I would gladly do it here since I see it as an
improvement. what say you Yair & Moti?
Line 11:
Line 12: static {
Line 13: mappings = new HashMap<Integer, EventNotificationMethod>();
Line 14: for (EventNotificationMethod value : values()) {
--
To view, visit http://gerrit.ovirt.org/22135
To unsubscribe, visit http://gerrit.ovirt.org/settings
Gerrit-MessageType: comment
Gerrit-Change-Id: I8b71c4e78bbdca3d02d2ac4ef419b9d3d7d58761
Gerrit-PatchSet: 12
Gerrit-Project: ovirt-engine
Gerrit-Branch: master
Gerrit-Owner: mooli tayer <mta...@redhat.com>
Gerrit-Reviewer: Barak Azulay <bazu...@redhat.com>
Gerrit-Reviewer: Eli Mesika <elimes...@gmail.com>
Gerrit-Reviewer: Juan Hernandez <juan.hernan...@redhat.com>
Gerrit-Reviewer: Martin Peřina <mper...@redhat.com>
Gerrit-Reviewer: Moti Asayag <masa...@redhat.com>
Gerrit-Reviewer: Yair Zaslavsky <yzasl...@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
