Shmuel Leib Melamud has posted comments on this change. Change subject: core: VmDeviceUtils refactoring ......................................................................
Patch Set 2: (3 comments) https://gerrit.ovirt.org/#/c/41620/2/backend/manager/modules/bll/src/main/java/org/ovirt/engine/core/bll/utils/VmDeviceUtils.java File backend/manager/modules/bll/src/main/java/org/ovirt/engine/core/bll/utils/VmDeviceUtils.java: Line 60: /* Line 61: * CD-ROM device Line 62: */ Line 63: Line 64: private static String getCdPath(String srcCdPath, String dstCdPath) { > please add a doc what this method do, the logic is not clear from the name Done Line 65: if (!StringUtils.isEmpty(dstCdPath)) { Line 66: return dstCdPath; Line 67: } else if (!StringUtils.isEmpty(srcCdPath)) { Line 68: return srcCdPath; Line 178: if (!hasSmartcardDevice(vmId)) { Line 179: addSmartcardDevice(vmId); Line 180: } Line 181: } else { Line 182: removeSmartcardDevices(vmId); > although this code is nicer, it will cause going to the db twice to get the This is not correct. If smartcardEnabled is true, the list of devices is queried only once in hasSmartcardDevice(). If smartcardEnabled is false, the list is also queried only once in removeSmartcardDevices(). Line 183: } Line 184: } Line 185: Line 186: /** Line 200: * Remove all smartcard devices from the VM. Line 201: * Line 202: * @param vmId Line 203: */ Line 204: public static void removeSmartcardDevices(Guid vmId) { > not sure if this should be public or not This method has clear purpose and well suited to be used when needed. And it is easy for any method to locate all the places where it is used, if desired. This class is designed to provide utility methods for everyone's usage. Only methods without clear purpose and universal usage are declared private here. If this method will be closed, it will be rather reimplemented in other places where it's needed and this is not good. Also, there are remove*() methods for other devices, that are already declared public and used from outside. So this method should be also made public just for consistency. Line 205: removeVmDevices(getSmartcardDevices(vmId)); Line 206: } Line 207: Line 208: /** -- To view, visit https://gerrit.ovirt.org/41620 To unsubscribe, visit https://gerrit.ovirt.org/settings Gerrit-MessageType: comment Gerrit-Change-Id: Iffa1ae3176b4082ef3b96277f6ea351e82f3e613 Gerrit-PatchSet: 2 Gerrit-Project: ovirt-engine Gerrit-Branch: master Gerrit-Owner: Shmuel Leib Melamud <smela...@redhat.com> Gerrit-Reviewer: Arik Hadas <aha...@redhat.com> Gerrit-Reviewer: Jenkins CI Gerrit-Reviewer: Omer Frenkel <ofren...@redhat.com> Gerrit-Reviewer: Shmuel Leib Melamud <smela...@redhat.com> Gerrit-Reviewer: Shmuel Melamud <smela...@redhat.com> Gerrit-Reviewer: automat...@ovirt.org Gerrit-HasComments: Yes _______________________________________________ Engine-patches mailing list Engine-patches@ovirt.org http://lists.ovirt.org/mailman/listinfo/engine-patches
