Liron Aravot has posted comments on this change.
Change subject: restapi,engine: fix usb mapping on update vm call
......................................................................
Patch Set 1: (3 inline comments)
....................................................
File
backend/manager/modules/restapi/jaxrs/src/main/java/org/ovirt/engine/api/restapi/resource/utils/UsbResourceUtils.java
Line 51: }
Line 52: else {
Line 53: if (!usb.isEnabled())
Line 54: return UsbPolicy.DISABLED;
Line 55: else {
this if (line 56) is duplication of the code in 44-45, for better readability
i'd just move it to the end of the method (all other parts seem to return value
- so we won't reach it otherwise and it will make it easier to understand in my
opinion by eliminating else/if clauses.
Line 56: if (usb.isSetType()) {
Line 57: UsbType usbType = UsbType.fromValue(usb.getType());
Line 58: return mapUsbTypeToPolicy(usbType);
Line 59: }
Line 56: if (usb.isSetType()) {
Line 57: UsbType usbType = UsbType.fromValue(usb.getType());
Line 58: return mapUsbTypeToPolicy(usbType);
Line 59: }
Line 60: else {
can't you avoid returning null? i'm pretty sure it will cause NPE sooner or
later or will cause us to have null check on every call to this method, a value
can be added to the enum if needed...
Line 61: return currentPolicy == UsbPolicy.DISABLED ? null
: currentPolicy;
Line 62: }
Line 63: }
Line 64: }
Line 63: }
Line 64: }
Line 65: }
Line 66:
Line 67: private static UsbPolicy mapUsbTypeToPolicy(UsbType usbType) {
this method is private, i'd remove the null check as you basically can't get
here with null as it seems to me. if you do get here with null or wrong value -
that's a bug that we better discover rather then continue the flow regulary..
Line 68: if (usbType == null)
Line 69: return null;
Line 70:
Line 71: switch (usbType) {
--
To view, visit http://gerrit.ovirt.org/9792
To unsubscribe, visit http://gerrit.ovirt.org/settings
Gerrit-MessageType: comment
Gerrit-Change-Id: If74bf6ede2fc0f932db8dfcd7951b6bcf90b7744
Gerrit-PatchSet: 1
Gerrit-Project: ovirt-engine
Gerrit-Branch: master
Gerrit-Owner: Arik Hadas <aha...@redhat.com>
Gerrit-Reviewer: Arik Hadas <aha...@redhat.com>
Gerrit-Reviewer: Michael Pasternak <mpast...@redhat.com>
Gerrit-Reviewer: Omer Frenkel <ofren...@redhat.com>
Gerrit-Reviewer: Roy Golan <rgo...@redhat.com>
_______________________________________________
Engine-patches mailing list
Engine-patches@ovirt.org
http://lists.ovirt.org/mailman/listinfo/engine-patches
