Shireesh Anjal has posted comments on this change.
Change subject: engine: Get Volume Advanced Details Query
......................................................................
Patch Set 21: (4 inline comments)
....................................................
File
backend/manager/modules/common/src/main/java/org/ovirt/engine/core/common/businessentities/gluster/GlusterVolumeAdvancedDetails.java
Line 28: public void copyDetailsFrom(GlusterVolumeAdvancedDetails entity) {
Line 29: for (BrickDetails entityBrick : entity.getBrickDetails()) {
Line 30: for (BrickDetails brick : getBrickDetails()) {
Line 31: if
(entityBrick.getBrickProperties().getBrickId().equals(brick.getBrickProperties().getBrickId()))
{
Line 32: brick = copyBrickDetails(brick, entityBrick);
What is the purpose of capturing the return value into the "brick" variable?
Line 33: }
Line 34: }
Line 35: }
Line 36: }
Line 34: }
Line 35: }
Line 36: }
Line 37:
Line 38: private BrickDetails copyBrickDetails(BrickDetails brick,
BrickDetails entityBrick) {
Would it sound better if the arguments are called "from" and "to" ?
Line 39: BrickProperties brickProperties =
entityBrick.getBrickProperties();
Line 40: brickProperties.setPid(brick.getBrickProperties().getPid());
Line 41: brickProperties.setPort(brick.getBrickProperties().getPort());
Line 42:
brickProperties.setStatus(brick.getBrickProperties().getStatus());
Line 40: brickProperties.setPid(brick.getBrickProperties().getPid());
Line 41: brickProperties.setPort(brick.getBrickProperties().getPort());
Line 42:
brickProperties.setStatus(brick.getBrickProperties().getStatus());
Line 43: brick.setBrickProperties(brickProperties);
Line 44: return brick;
What's the point in returning the argument "brick" itself?
Line 45: }
Line 46:
Line 47: public void copyClientsFrom(GlusterVolumeAdvancedDetails entity) {
Line 48: for (BrickDetails entityBrick : entity.getBrickDetails()) {
....................................................
File
backend/manager/modules/dal/src/test/java/org/ovirt/engine/core/dao/gluster/GlusterVolumeDaoTest.java
Line 267: assertEquals(volumeAfter, existingReplVol);
Line 268: }
Line 269:
Line 270: @Test
Line 271: public void testGetByVolumesOption() {
The name of the test method should also change to testGetVolumesByOption()
Line 272: List<GlusterVolumeEntity> volumes =
dao.getVolumesByOption(CLUSTER_ID, GlusterStatus.UP, "nfs.disable", "off");
Line 273:
Line 274: assertTrue(volumes != null);
Line 275: assertTrue(volumes.contains(existingReplVol));
--
To view, visit http://gerrit.ovirt.org/7807
To unsubscribe, visit http://gerrit.ovirt.org/settings
Gerrit-MessageType: comment
Gerrit-Change-Id: If6b590eaebeb1d06b7278300d5e12b2dab9eb093
Gerrit-PatchSet: 21
Gerrit-Project: ovirt-engine
Gerrit-Branch: master
Gerrit-Owner: Dhandapani Gopal <[email protected]>
Gerrit-Reviewer: Dhandapani Gopal <[email protected]>
Gerrit-Reviewer: Kanagaraj M <[email protected]>
Gerrit-Reviewer: Mike Kolesnik <[email protected]>
Gerrit-Reviewer: Moti Asayag <[email protected]>
Gerrit-Reviewer: Omer Frenkel <[email protected]>
Gerrit-Reviewer: Selvasundaram <[email protected]>
Gerrit-Reviewer: Shireesh Anjal <[email protected]>
Gerrit-Reviewer: Yair Zaslavsky <[email protected]>
Gerrit-Reviewer: oVirt Jenkins CI Server
_______________________________________________
Engine-patches mailing list
[email protected]
http://lists.ovirt.org/mailman/listinfo/engine-patches
