https://bz.apache.org/bugzilla/show_bug.cgi?id=60826
Tim Allison <[email protected]> changed: What |Removed |Added ---------------------------------------------------------------------------- Resolution|--- |FIXED Status|REOPENED |RESOLVED --- Comment #7 from Tim Allison <[email protected]> --- Javen, Many, many thanks for your careful code review! Changes made r1787320. Let me know if you see anything else worth changing. Details: Global: Replace 3.15-beta3 with 3.16-beta3 >Done XSSFBHeaderFooter: headerFooterHelper should either be a static final member, or even better, a non-instantiable (static) class with static methods. >Changed to static final. Y, we should make that a non-instantiable static class. I was working with what was already in POI. Docs: use https URLs when possible >I found one in package.html, should I also change them in the license headers? Docs: spell out full org.apache.poi... rather than abbreviating to o.a.p >Done. XSSFBRelation: can getContents be moved up to a superclass (POIXMLRelation)? There isn't any logic here that's specific to XSSFB. >Done, and removed from XSSFRelation XSSFBRichStr: we should try to make this implement the RichString interface later. >Agreed. XSSFBRecordType: lookup can have constant lookup time if we use a Map at the expense of some memory. >Done. XSSFBRichTextString: add @org.apache.poi.util.NotImplemented annotation to stubbed functions that haven't been implemented yet. >Done. Global: add @since 3.16-beta3 javadoc to all new classes >Done. XSSFBCellHeader: formatAddressAsString can be implemented using new CellAddress(int row, int col).formatAsString(). >As part of premature optimization, I was trying to cut down on new objects. Turns out that was vestigial to an earlier draft, and I've removed it. XSSFBSharedStringsTable: change constructor Javadoc to @since 3.16-beta3. >deleted. Relying on @since 3.16-beta3 at class level. XSSFBSharedStringsTable: should getItems return an unmodifiableList or a copy of the list? >Yes...always the tradeoff of security vs efficiency. I've modified it to make a copy. XSSFBCellRange: "public static final int length" is the standard modifier order. See http://stackoverflow.com/a/10299123/2683399 for order >Did a global replace in xssf. I suspect we could do this across the project. XSSFBCellRange: this should have tighter integration with o.a.p.ss.util.AreaReference in the future, but AreaReference probably needs some cleanup first. >Agreed. XSSFBCommentsTable: should the natural ordering of CellAddresses be implemented into the CellAddress class itself instead as CellAddressComparator inner class? Or perhaps as a standalone class if both row-major and column-major ordering is needed. >Duh. CellAddress already naturally sorts row-major. I removed the CellAddressComparator in XSSFBCommentsTable. XSSFEventBasedExcelExtractor: any reason for elevating visibility from private? Was this just a quick fix to get the code to work? Is the performance acceptable if you add accessors to private members? If not, does protected work? >All back to private with access via getters. XSSFBEventBasedExcelExtractor: should we log caught and suppressed exceptions to the POILogger rather than stderr? >Done in both XSSFBEvent... and XSSFEvent... TestExtractorFactory: replace assertTrue(String.contains(String)) with POITestCase.assertContains(String haystack, String needle) >Done throughout TestExtractorFactory TestXSSFBReader: import assertContains from POITestCase rather than redefining. >Gah. Thank you. Done. TestXSSFBEventBasedExcelExtractor: testShapes uses String.indexOf(String) > -1. Consider using or adding something to POITestCase to make the purpose clearer >Used POITestCase and fixed TestXSSFEventBasedExcelExtractor throughout as well. -- You are receiving this mail because: You are the assignee for the bug. --------------------------------------------------------------------- To unsubscribe, e-mail: [email protected] For additional commands, e-mail: [email protected]
