Sahina Bose has posted comments on this change. Change subject: engine : volume profile export to pdf command ......................................................................
Patch Set 17: (11 comments) http://gerrit.ovirt.org/#/c/28000/17/backend/manager/dependencies/pom.xml File backend/manager/dependencies/pom.xml: Line 233: Line 234: <dependency> Line 235: <groupId>com.itextpdf</groupId> Line 236: <artifactId>itextpdf</artifactId> Line 237: <version>5.5.1</version> you should define this version in properties section of pom.xml at project root Line 238: </dependency> Line 239: Line 240: </dependencies> Line 241: http://gerrit.ovirt.org/#/c/28000/17/backend/manager/modules/bll/pom.xml File backend/manager/modules/bll/pom.xml: Line 161: </dependency> Line 162: <dependency> Line 163: <groupId>com.itextpdf</groupId> Line 164: <artifactId>itextpdf</artifactId> Line 165: <version>5.5.1</version> same comment here as backend/../pom.xml Line 166: <scope>provided</scope> Line 167: </dependency> Line 168: </dependencies> Line 169: Line 162: <dependency> Line 163: <groupId>com.itextpdf</groupId> Line 164: <artifactId>itextpdf</artifactId> Line 165: <version>5.5.1</version> Line 166: <scope>provided</scope> Is this scope required? Line 167: </dependency> Line 168: </dependencies> Line 169: Line 170: <build> http://gerrit.ovirt.org/#/c/28000/17/backend/manager/modules/bll/src/main/java/org/ovirt/engine/core/bll/gluster/CreatePdf.java File backend/manager/modules/bll/src/main/java/org/ovirt/engine/core/bll/gluster/CreatePdf.java: Line 13: import com.itextpdf.text.Phrase; Line 14: import com.itextpdf.text.pdf.PdfPCell; Line 15: import com.itextpdf.text.pdf.PdfPTable; Line 16: Line 17: public class CreatePdf { Too generic. Also can you rename the class so that it's not like a method? Line 18: public static final Font headingFont = new Font(Font.FontFamily.TIMES_ROMAN, 18, Font.BOLD); Line 19: public static final Font subHeadingFont = new Font(Font.FontFamily.TIMES_ROMAN, 16, Font.BOLD); Line 20: public static final Font paragraphFont = new Font(Font.FontFamily.TIMES_ROMAN, 12, Font.NORMAL); Line 21: Line 49: for(String columnHeader : columnHeaders) { Line 50: PdfPCell tableColumnCell = new PdfPCell(new Phrase(columnHeader)); Line 51: tableColumnCell.setHorizontalAlignment(Element.ALIGN_CENTER); Line 52: table.addCell(tableColumnCell); Line 53: } code is repeated. please refactor Line 54: table.setHeaderRows(noOfHeaderRows); Line 55: for (String item : data) { Line 56: table.addCell(item); Line 57: } Line 53: } Line 54: table.setHeaderRows(noOfHeaderRows); Line 55: for (String item : data) { Line 56: table.addCell(item); Line 57: } same for above code..exactly same as populateDataIntoTable Line 58: return table; Line 59: } Line 60: Line 61: public PdfPTable populateDataIntoTable(PdfPTable table, String[] data) { Line 64: } Line 65: return table; Line 66: } Line 67: Line 68: public List createList(String[] listPoints) { does this need to be public? Line 69: List textList = new List(false, false, 10); Line 70: for (String listPoint : listPoints) { Line 71: textList.add(new ListItem(listPoint)); Line 72: } http://gerrit.ovirt.org/#/c/28000/17/backend/manager/modules/bll/src/main/java/org/ovirt/engine/core/bll/gluster/GlusterVolumeProfilePdfExportCommand.java File backend/manager/modules/bll/src/main/java/org/ovirt/engine/core/bll/gluster/GlusterVolumeProfilePdfExportCommand.java: Line 88: Line 89: List<String> tableContents = lineariseStats(profile.getProfileStats().get(0)); Line 90: profileDetails.add(createPdf.createTableWithData(profileColumnHeaders, 1, tableContents.toArray(new String[tableContents.size()]))); Line 91: Line 92: Pair<Integer, String> runTimeConverted = profile.getProfileStats().get(1).getDurationFormatted(); possible NPEs and IndexOutofBoundExceptions Line 93: Pair<Integer, String> intervalRunTimeConverted = profile.getProfileStats().get(0).getDurationFormatted(); Line 94: Pair<SizeUnit, Double> dataReadInCurrentInterval = SizeConverter.autoConvert(profile.getProfileStats().get(1).getTotalRead(), SizeUnit.BYTES); Line 95: Pair<SizeUnit, Double> dataRead = SizeConverter.autoConvert(profile.getProfileStats().get(0).getTotalRead(), SizeUnit.BYTES); Line 96: Pair<SizeUnit, Double> dataWrittenInCurrentInterval = SizeConverter.autoConvert(profile.getProfileStats().get(1).getTotalWrite(), SizeUnit.BYTES); Line 115: Line 116: } Line 117: Line 118: private List<String> lineariseStats(StatsInfo statsInfo) { Line 119: return getLinearStrings(statsInfo.getFopStats()); what about the blockStats? Line 120: } Line 121: Line 122: private List<String> getLinearStrings(List<FopStats> fopsStats) { Line 123: List<String> fopStatsStringified = new ArrayList<String>(); Line 128: + fopStats.getMaxLatencyFormatted().getSecond()); Line 129: fopStatsStringified.add(fopStats.getMinLatencyFormatted().getFirst() Line 130: + fopStats.getMinLatencyFormatted().getSecond()); Line 131: fopStatsStringified.add(fopStats.getAvgLatencyFormatted().getFirst() Line 132: + fopStats.getAvgLatencyFormatted().getSecond()); Don't you need to separate each fopStats? Line 133: } Line 134: return fopStatsStringified; Line 135: } Line 136: http://gerrit.ovirt.org/#/c/28000/17/backend/manager/modules/common/pom.xml File backend/manager/modules/common/pom.xml: Line 27: Line 28: <dependency> Line 29: <groupId>com.itextpdf</groupId> Line 30: <artifactId>itextpdf</artifactId> Line 31: <version>5.5.1</version> use external version property Line 32: <scope>provided</scope> Line 33: </dependency> Line 34: <dependency> Line 35: <groupId>org.jboss.spec.javax.ejb</groupId> -- To view, visit http://gerrit.ovirt.org/28000 To unsubscribe, visit http://gerrit.ovirt.org/settings Gerrit-MessageType: comment Gerrit-Change-Id: I208a77456487a5f735948d6cf9fec54f004b6e89 Gerrit-PatchSet: 17 Gerrit-Project: ovirt-engine Gerrit-Branch: master Gerrit-Owner: anmolbabu <anb...@redhat.com> Gerrit-Reviewer: Alexander Wels <aw...@redhat.com> Gerrit-Reviewer: Kanagaraj M <kmayi...@redhat.com> Gerrit-Reviewer: Sahina Bose <sab...@redhat.com> Gerrit-Reviewer: Shubhendu Tripathi <shtri...@redhat.com> Gerrit-Reviewer: anmolbabu <anb...@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
