Juan Hernandez has posted comments on this change.
Change subject: restapi: Snapshot Refactoring
......................................................................
Patch Set 1: (13 inline comments)
I did what I can with my limited knowledge of RESTAPI. Most of my comments are
just Java things. Hope this helps!
....................................................
File
backend/manager/modules/restapi/interface/definition/src/main/java/org/ovirt/engine/api/model/SnapshotStatus.java
Line 16: return null;
Not very important in this case, but I think in general is bad practice to
swallow exceptions. Can you send it to the log with an error message? Something
like this:
log.error("Value \"" + v + "\" is not a valid snapshot status, will return
null.", exception);
....................................................
File
backend/manager/modules/restapi/interface/definition/src/main/java/org/ovirt/engine/api/model/SnapshotType.java
Line 18: return null;
Please try not to swallow the exception.
....................................................
File
backend/manager/modules/restapi/interface/definition/src/main/java/org/ovirt/engine/api/resource/SnapshotElementsResource.java
Line 13: public interface SnapshotElementsResource<D extends BaseResource, C
extends BaseResources> {
What are the meanings of D and C here? D for "data" and C for "collection".
Could use longer names or comments.
....................................................
File
backend/manager/modules/restapi/jaxrs/src/main/java/org/ovirt/engine/api/restapi/resource/BackendSnapshotCdRomsResource.java
Line 28: // TODO Auto-generated method stub
Does this need additional code? If it do please remove the comment generated by
the IDE and add a comment explaining what is needed.
....................................................
File
backend/manager/modules/restapi/jaxrs/src/main/java/org/ovirt/engine/api/restapi/resource/BackendSnapshotDiskResource.java
Line 10: // TODO Auto-generated method stub
Please explain what is missing here.
....................................................
File
backend/manager/modules/restapi/jaxrs/src/main/java/org/ovirt/engine/api/restapi/resource/BackendSnapshotDisksResource.java
Line 31: return null;
Is it ok to return null here?
....................................................
File
backend/manager/modules/restapi/jaxrs/src/main/java/org/ovirt/engine/api/restapi/resource/BackendSnapshotNicResource.java
Line 11: return null;
Please remove the autogenerated comment and explain what is missing.
....................................................
File
backend/manager/modules/restapi/jaxrs/src/main/java/org/ovirt/engine/api/restapi/resource/BackendSnapshotNicsResource.java
Line 31: return null;
Is it ok to return null here? If so please comment why.
....................................................
File
backend/manager/modules/restapi/jaxrs/src/main/java/org/ovirt/engine/api/restapi/resource/BackendSnapshotResource.java
Line 48: notFound();
Shouldn't this be "entity = notFound()" ?
....................................................
File
backend/manager/modules/restapi/jaxrs/src/main/java/org/ovirt/engine/api/restapi/resource/BackendSnapshotRomResource.java
Line 11: return null;
Plese remove the autogereated comment and explain if returning null is ok here.
....................................................
File
backend/manager/modules/restapi/jaxrs/src/main/java/org/ovirt/engine/api/restapi/resource/BackendSnapshotsResource.java
Line 114: SnapshotIdResolver() {}
No need for this default constructor.
....................................................
File
backend/manager/modules/restapi/types/src/main/java/org/ovirt/engine/api/restapi/types/SnapshotMapper.java
Line 39: return null;
In situations like this I would send a message to the log. Something like this:
log.warn("Don't know how to map snapshot type \"" + snapshotType + "\".");
Line 53: return null;
Same as before.
--
To view, visit http://gerrit.ovirt.org/3731
To unsubscribe, visit http://gerrit.ovirt.org/settings
Gerrit-MessageType: comment
Gerrit-Change-Id: Idca90053f20c77b51698fedaca23b339a85920c8
Gerrit-PatchSet: 1
Gerrit-Project: ovirt-engine
Gerrit-Branch: master
Gerrit-Owner: Ori Liel <[email protected]>
Gerrit-Reviewer: Juan Hernandez <[email protected]>
Gerrit-Reviewer: Ori Liel <[email protected]>
_______________________________________________
Engine-patches mailing list
[email protected]
http://lists.ovirt.org/mailman/listinfo/engine-patches
