Bug 58402

Summary: Rework AreaReference implementation details and tests
Product: POI Reporter: Javen O'Neal <onealj>
Component: SS CommonAssignee: POI Developers List <dev>
Status: NEW ---    
Severity: enhancement CC: dtn-asfbugs
Priority: P2    
Version: 3.13-dev   
Target Milestone: ---   
Hardware: PC   
OS: Linux   
Bug Depends on: 56328    
Bug Blocks:    
Attachments: AreaReference.java patch
AreaReference.java.patch

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.