|Summary:||Rework AreaReference implementation details and tests|
|Product:||POI||Reporter:||Javen O'Neal <onealj>|
|Component:||SS Common||Assignee:||POI Developers List <dev>|
|Bug Depends on:||56328|
Description Javen O'Neal 2015-09-13 10:31:57 UTC
Created attachment 33099 [details] AreaReference.java patch From Bug 56328 (implemented in poi-3.13-beta1) r1685101 AreaReference's SpreadsheetVersion instance variable, _version is private and is unable to be modified outside of the constructors (no setters provided). This field should probably be final like the rest of the instance variables in this class. AreaReference(CellReference, CellReference) currently assumes SpreadsheetVersion is EXCEL97. I deprecated this constructor in favor of AreaReference(CellReference, CellReference, SpreadsheetVersion). I wasn't sure what the preferred argument order was here, since the other constructor is AreaReference(String, SpreadsheetVersion), but the static methods list SpreadsheetVersion as the first argument. Your call. I added a few other lines of defensive code to prevent NPEs. One thing lead to another and I wrote unit tests for nearly all of AreaReference. Note: many of these tests currently fail. In general, this class should probably either delegate to a common cell reference string parser or remove functions that operate on strings. It seems like allowing Strings that do not match "[A-Z]+[0-9]+(:[A-Z]+[0-9]+)?" or perhaps even "[A-Z]+[0-9]+:[A-Z]+[0-9]+" should raise an IllegalArgumentException--it shouldn't be AreaReference's responsibility to strip off sheet information and know sheet (and external workbook) naming rules. Note: this patch requires further work before it can be committed.
Comment 1 Javen O'Neal 2015-10-23 10:09:45 UTC
Applied some of the internal changes from attachment 33099 [details] in r1710163.
Comment 2 Javen O'Neal 2015-10-23 11:05:27 UTC
Created attachment 33198 [details] AreaReference.java.patch Rebased to r1710170. In this patch: * deprecate AreaReference(CellReference, CellReference), in favor of AreaReference(CellReference, CellReference, SpreadsheetVersion) * use static constants where ever possible * pre-allocate a fixed-length list for getAllReferencedCells * add many much-needed unit tests Remaining work: fix either the unit tests or the code For example, AreaReference("A1:B65536", EXCEL97).isWholeColumn() is false because it doesn't include dollar signs. AreaReference("A:B").isWholeColumn() is false because it doesn't include numbers. This doesn't seem like the correct behavior.