Bug 58402 - Rework AreaReference implementation details and tests
Summary: Rework AreaReference implementation details and tests
Status: NEW
Alias: None
Product: POI
Classification: Unclassified
Component: SS Common (show other bugs)
Version: 3.13-dev
Hardware: PC Linux
: P2 enhancement (vote)
Target Milestone: ---
Assignee: POI Developers List
Depends on: 56328
  Show dependency tree
Reported: 2015-09-13 10:31 UTC by Javen O'Neal
Modified: 2015-12-12 13:42 UTC (History)
1 user (show)

AreaReference.java patch (17.16 KB, patch)
2015-09-13 10:31 UTC, Javen O'Neal
Details | Diff
AreaReference.java.patch (16.55 KB, patch)
2015-10-23 11:05 UTC, Javen O'Neal
Details | Diff

Note You need to log in before you can comment on or make changes to this bug.
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]

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.