Bug 58403 - Require SpreadsheetVersion on CellRangeAddress.isFullColumnRange and isFullRowRange
Summary: Require SpreadsheetVersion on CellRangeAddress.isFullColumnRange and isFullRo...
Status: NEW
Alias: None
Product: POI
Classification: Unclassified
Component: SS Common (show other bugs)
Version: unspecified
Hardware: PC Linux
: P2 enhancement (vote)
Target Milestone: ---
Assignee: POI Developers List
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2015-09-13 11:32 UTC by Javen O'Neal
Modified: 2015-10-31 08:36 UTC (History)
0 users



Attachments
CellRangeAddressBase.java patch plus unit tests (8.33 KB, patch)
2015-09-13 11:32 UTC, Javen O'Neal
Details | Diff
CellRangeAddressBase.java patch plus unit tests (7.20 KB, patch)
2015-10-31 08:33 UTC, Javen O'Neal
Details | Diff
CellRangeAddressBase.java patch plus unit tests (7.21 KB, patch)
2015-10-31 08:36 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 11:32:17 UTC
Created attachment 33100 [details]
CellRangeAddressBase.java patch plus unit tests

This patch resolves a couple TODO's in org.apache.poi.ss.util.CellRangeAddress.

I see three options here:
1) Require the caller to supply the spreadsheet version as an argument to isFullColumnRange and isFullRowRange, deprecate the zero-argument version of these methods.
2) Add a SpreadsheetVersion instance variable to this class, set by the constructor (either a final instance variable or with a setter).
3) Let the implementing class (org.apache.poi.hssf.util.CellRangeAddress) override the zero-argument isFullColumnRange to specify the SpreadsheetVersion.

The first method seems best, as it has the least impact on existing projects, reduces amount of RAM consumed by this object, and resembles other classes that have been modified with SpreadsheetVersion. The downside is the caller (rather than creator for Option 2) needs to know the SpreadsheetVersion.

There seems to be a lot of overlap between AreaReference and CellRangeAddress
org.apache.poi.ss.util.AreaReference[1]. In r1685101, AreaReference took the direction of #2 above, adding a SpreadsheetVersion instance variable and deprecating constructors that don't specify the spreadsheet version. AreaReference#isWholeColumnReference() seems to behave similarly to CellRangeAddress#isFullColumnRange. Oddly, AreaReference doesn't include a isWholeRowReference method. CellRangeAddressBase stores 4 ints while AreaReference stores 2 CellReferences. Key difference: References include absolute/relative fields while Addresses do not.

Seems like some refactoring is needed here, perhaps merging these classes or having these classes subclassed from a common class. Anyone have any strong opinions on what direction should be taken?

[1] https://svn.apache.org/viewvc/poi/trunk/src/java/org/apache/poi/ss/util/AreaReference.java?revision=1685101&view=markup#l194
Comment 1 Javen O'Neal 2015-10-31 08:33:30 UTC
Created attachment 33237 [details]
CellRangeAddressBase.java patch plus unit tests

Rebasing patch due to conflicts with bug 58441: define equals method for CellRangeAddressBase, needed for a different bug.

Also fixed broken hashCode test.
Comment 2 Javen O'Neal 2015-10-31 08:36:25 UTC
Created attachment 33238 [details]
CellRangeAddressBase.java patch plus unit tests

Fix whitespace and hashCode implementation.