Martin Mucha has posted comments on this change. Change subject: core: Added DAO for HostNetworkQos entities ......................................................................
Patch Set 1: Code-Review-1 (4 comments) http://gerrit.ovirt.org/#/c/34121/1/backend/manager/modules/bll/src/main/java/org/ovirt/engine/core/bll/QosQueryBase.java File backend/manager/modules/bll/src/main/java/org/ovirt/engine/core/bll/QosQueryBase.java: Line 10: private QosDao<?> qosDao what's the point of this field? It seems like caching (which is inherently wrong), but it's not used, since its value is calculated in getQosDao methods always anyways. So: * at least, this field can become a local variable. I know it's not your code, but this probably should be improved. Line 17: QosType qosType = getParameters().getQosType(); Line 18: if (qosType == null) { Line 19: return getDbFacade().getQosBaseDao(); Line 20: } Line 21: switch (qosType) { this switch should be encapsulated in separate method. Line 22: case STORAGE: Line 23: qosDao = getDbFacade().getStorageQosDao(); Line 24: break; Line 25: case CPU: Line 30: break; Line 31: case HOSTNETWORK: Line 32: qosDao = getDbFacade().getHostNetworkQosDao(); Line 33: break; Line 34: default: notice that in default case null will be returned causing NPE in all usages. It's better to throw exception closest to the problem, instead doing debug logging and firing runtime exception (NPE) in caller. proper way here would be: * throw exception in default case * get rid of field/variable qosDao, and from each case (except of default one) immediately return obtained value. ad multi exit-points: I know, that some 'experts' recommends single return point from methods, but there's no real argumentation for that. It just improves cyclomatic complexity metric at a cost of adding / recycling variable (ie. code is worse, but it's cyclomatic complexity metric is better). Especially with no postconditions used, single exit place is no benefit at all. Line 35: log.debugFormat("Not handled QoS type: {0}", qosType); Line 36: break; Line 37: } Line 38: return qosDao; http://gerrit.ovirt.org/#/c/34121/1/backend/manager/modules/dal/src/main/java/org/ovirt/engine/core/dao/qos/AllQosBaseDaoFacadeImpl.java File backend/manager/modules/dal/src/main/java/org/ovirt/engine/core/dao/qos/AllQosBaseDaoFacadeImpl.java: Line 54: case NETWORK: Line 55: return NetworkQoSDaoFacadeImpl.NetworkQosDaoDbFacadaeImplMapper.MAPPER.createQosEntity(rs); Line 56: case HOSTNETWORK: Line 57: return HostNetworkQosDaoDbFacadeImpl.HostNetworkQosDaoDbFacadaeImplMapper.MAPPER.createQosEntity(rs); Line 58: default: there is code style issue: once we're immediately returning from case, once we're using break. Again, I know this is not your change, but you've touched this file and this change is minimal, so it shouldn't be a problem to do it. Line 59: log.debugFormat("not handled/missing qos_type", qosType); Line 60: break; Line 61: } Line 62: -- To view, visit http://gerrit.ovirt.org/34121 To unsubscribe, visit http://gerrit.ovirt.org/settings Gerrit-MessageType: comment Gerrit-Change-Id: I90535167d324c80aa9de7a735a010818ab45e428 Gerrit-PatchSet: 1 Gerrit-Project: ovirt-engine Gerrit-Branch: master Gerrit-Owner: Lior Vernia <lver...@redhat.com> Gerrit-Reviewer: Alona Kaplan <alkap...@redhat.com> Gerrit-Reviewer: Lior Vernia <lver...@redhat.com> Gerrit-Reviewer: Martin Mucha <mmu...@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
