Martin Mucha has posted comments on this change. Change subject: core: simplified refreshVdsmFileList unsuccessful call to vdsm as well as returning null are considered to be invalid return values. Taking advantage of it, if vds call is unsuccessful, it's pretended as if null was returned instead simplifying further pr ......................................................................
Patch Set 4: (2 comments) https://gerrit.ovirt.org/#/c/41275/4/backend/manager/modules/bll/src/main/java/org/ovirt/engine/core/bll/IsoDomainListSyncronizer.java File backend/manager/modules/bll/src/main/java/org/ovirt/engine/core/bll/IsoDomainListSyncronizer.java: Line 677: return refreshVdsmFileList(repoStoragePoolId, Line 678: repoStorageDomainId, Line 679: fileListRefreshed, Line 680: ImageFileType.ISO, Line 681: fileStatsFromVDSReturnValue(fileStats)); > ok, but why to extract it from refreshVdsmFileList? you can do it at the be I tried to peek into following patches and it seems, that there will be code interresting in map passed in VDSReturnValue sooner thatn calling 'refresh…'. In that case everyone would need to verify it's potential nullity and then content itself. Also, from OO desing point, I think it's otherway around. Method should get ONLY minimum information it needs, nothing extra. Unless, with one eye closed, it simplifies the code. I think, that here it would complicate the situation a little (when looking into following patches), so there's no benefit passing unneeded extra information into method. Line 682: } Line 683: Line 684: private boolean refreshVdsmFileList(Guid repoStoragePoolId, Line 685: Guid repoStorageDomainId, Line 717: } Line 718: Line 719: @SuppressWarnings("unchecked") Line 720: Map<String, Map<String, Object>> result = (Map<String, Map<String, Object>>) fileStats.getReturnValue(); Line 721: return result; > please inline, no need for the 'result' variable (the annotation can be dec defining this annotation on method is rather troublesome — there won't be any warnings during writing future modifications. Defining this annotation on variable limits 'suppress scope'. That's the need for the result variable. I don't want to suppress warnings for methods, this is not good technique. Do you prefer warning instead of 'unneeded' variable? If so, I can drop that variable and suppression. Line 722: } Line 723: Line 724: public interface FileListRefreshed { Line 725: void onFileListRefreshed(Guid poolId, Set<String> isoList); -- To view, visit https://gerrit.ovirt.org/41275 To unsubscribe, visit https://gerrit.ovirt.org/settings Gerrit-MessageType: comment Gerrit-Change-Id: Ic8b53afcd6c0e87f4f206a60735cecf9a51480c5 Gerrit-PatchSet: 4 Gerrit-Project: ovirt-engine Gerrit-Branch: master Gerrit-Owner: Martin Mucha <[email protected]> Gerrit-Reviewer: Arik Hadas <[email protected]> Gerrit-Reviewer: Jenkins CI Gerrit-Reviewer: Martin Mucha <[email protected]> Gerrit-Reviewer: [email protected] Gerrit-HasComments: Yes _______________________________________________ Engine-patches mailing list [email protected] http://lists.ovirt.org/mailman/listinfo/engine-patches
