Review EscherContainerRecord#getChildRecords() call sites for unnecessary work
This started off as wanting to add the EscherContainerRecord#getChildCount() function in order to do an efficient check for how many children the container has. This was desirable in new code for editing HSSF pictures. The existing option of calling getChildRecords().size() was undesirable as this requires a list copy first.
In the process of finding call sites that would benefit from replacing getChildRecords().size(), I realized that several other patterns would benefit from eliminating a copy, such as iterating over the children in a for-each loop, and indexed access to specific children.
git-svn-id: https://svn.apache.org/repos/asf/poi/trunk@1887020 13f79535-47bb-0310-9956-ffa450edef68
diff --git a/src/java/org/apache/poi/ddf/EscherContainerRecord.java b/src/java/org/apache/poi/ddf/EscherContainerRecord.java
index 474b24a..bbe7efa 100644
--- a/src/java/org/apache/poi/ddf/EscherContainerRecord.java
+++ b/src/java/org/apache/poi/ddf/EscherContainerRecord.java
@@ -158,6 +158,10 @@
return new ArrayList<>(_childRecords);
}
+ public int getChildCount() {
+ return _childRecords.size();
+ }
+
/**
* @return an iterator over the child records
*/
diff --git a/src/java/org/apache/poi/hssf/model/InternalWorkbook.java b/src/java/org/apache/poi/hssf/model/InternalWorkbook.java
index b6d60ec..c5703df 100644
--- a/src/java/org/apache/poi/hssf/model/InternalWorkbook.java
+++ b/src/java/org/apache/poi/hssf/model/InternalWorkbook.java
@@ -1880,7 +1880,7 @@
DrawingManager2 dm = new DrawingManager2(dgg);
if(bStore != null){
- for(EscherRecord bs : bStore.getChildRecords()){
+ for (EscherRecord bs : bStore) {
if(bs instanceof EscherBSERecord) {
escherBSERecords.add((EscherBSERecord)bs);
}
diff --git a/src/java/org/apache/poi/hssf/usermodel/HSSFShape.java b/src/java/org/apache/poi/hssf/usermodel/HSSFShape.java
index 92d66e1..964356f 100644
--- a/src/java/org/apache/poi/hssf/usermodel/HSSFShape.java
+++ b/src/java/org/apache/poi/hssf/usermodel/HSSFShape.java
@@ -186,9 +186,9 @@
throw new IllegalArgumentException("Must use client anchors for shapes directly attached to sheet.");
EscherClientAnchorRecord anch = _escherContainer.getChildById(EscherClientAnchorRecord.RECORD_ID);
if (null != anch) {
- for (i=0; i< _escherContainer.getChildRecords().size(); i++){
+ for (i=0; i< _escherContainer.getChildCount(); i++){
if (_escherContainer.getChild(i).getRecordId() == EscherClientAnchorRecord.RECORD_ID){
- if (i != _escherContainer.getChildRecords().size() -1){
+ if (i != _escherContainer.getChildCount() -1){
recordId = _escherContainer.getChild(i+1).getRecordId();
}
}
@@ -200,9 +200,9 @@
throw new IllegalArgumentException("Must use child anchors for shapes attached to groups.");
EscherChildAnchorRecord anch = _escherContainer.getChildById(EscherChildAnchorRecord.RECORD_ID);
if (null != anch) {
- for (i=0; i< _escherContainer.getChildRecords().size(); i++){
+ for (i=0; i< _escherContainer.getChildCount(); i++){
if (_escherContainer.getChild(i).getRecordId() == EscherChildAnchorRecord.RECORD_ID){
- if (i != _escherContainer.getChildRecords().size() -1){
+ if (i != _escherContainer.getChildCount() -1){
recordId = _escherContainer.getChild(i+1).getRecordId();
}
}
diff --git a/src/java/org/apache/poi/hssf/usermodel/HSSFShapeGroup.java b/src/java/org/apache/poi/hssf/usermodel/HSSFShapeGroup.java
index 4652693..b8001fc 100644
--- a/src/java/org/apache/poi/hssf/usermodel/HSSFShapeGroup.java
+++ b/src/java/org/apache/poi/hssf/usermodel/HSSFShapeGroup.java
@@ -54,7 +54,7 @@
// read internal and external coordinates from spgrContainer
EscherContainerRecord spContainer = spgrContainer.getChildContainers().get(0);
_spgrRecord = (EscherSpgrRecord) spContainer.getChild(0);
- for (EscherRecord ch : spContainer.getChildRecords()) {
+ for (EscherRecord ch : spContainer) {
switch (EscherRecordTypes.forTypeID(ch.getRecordId())) {
case SPGR:
break;
diff --git a/src/scratchpad/src/org/apache/poi/hslf/dev/SlideShowRecordDumper.java b/src/scratchpad/src/org/apache/poi/hslf/dev/SlideShowRecordDumper.java
index 8463080..cff59a7 100644
--- a/src/scratchpad/src/org/apache/poi/hslf/dev/SlideShowRecordDumper.java
+++ b/src/scratchpad/src/org/apache/poi/hslf/dev/SlideShowRecordDumper.java
@@ -213,11 +213,10 @@
ps.println(ind + " options: 0x" + HexDump.toHex( ecr.getOptions() ));
ps.println(ind + " recordId: 0x" + HexDump.toHex( ecr.getRecordId() ));
- List<EscherRecord> childRecords = ecr.getChildRecords();
- ps.println(ind + " numchildren: " + childRecords.size());
+ ps.println(ind + " numchildren: " + ecr.getChildCount());
ps.println(ind + " children: ");
int count = 0;
- for ( EscherRecord record : childRecords ) {
+ for ( EscherRecord record : ecr ) {
ps.println(ind + " Child " + count + ":");
printEscherRecord(record, indent+1);
count++;
diff --git a/src/scratchpad/src/org/apache/poi/hslf/usermodel/HSLFFill.java b/src/scratchpad/src/org/apache/poi/hslf/usermodel/HSLFFill.java
index 5275669..bb2c52b 100644
--- a/src/scratchpad/src/org/apache/poi/hslf/usermodel/HSLFFill.java
+++ b/src/scratchpad/src/org/apache/poi/hslf/usermodel/HSLFFill.java
@@ -459,8 +459,7 @@
LOG.atDebug().log("EscherContainerRecord.BSTORE_CONTAINER was not found ");
return null;
}
- List<EscherRecord> lst = bstore.getChildRecords();
- return (EscherBSERecord)lst.get(idx-1);
+ return (EscherBSERecord) bstore.getChild(idx-1);
}
/**
@@ -563,12 +562,11 @@
EscherContainerRecord dggContainer = doc.getPPDrawingGroup().getDggContainer();
EscherContainerRecord bstore = HSLFShape.getEscherChild(dggContainer, EscherContainerRecord.BSTORE_CONTAINER);
- List<EscherRecord> lst = bstore.getChildRecords();
int idx = p.getPropertyValue();
if (idx == 0){
LOG.atWarn().log("no reference to picture data found ");
} else {
- EscherBSERecord bse = (EscherBSERecord)lst.get(idx - 1);
+ EscherBSERecord bse = (EscherBSERecord) bstore.getChild(idx - 1);
for (HSLFPictureData pd : pict) {
// Reference equals is safe because these BSE belong to the same slideshow
diff --git a/src/scratchpad/src/org/apache/poi/hslf/usermodel/HSLFPictureShape.java b/src/scratchpad/src/org/apache/poi/hslf/usermodel/HSLFPictureShape.java
index 548fdff..6aba4b2 100644
--- a/src/scratchpad/src/org/apache/poi/hslf/usermodel/HSLFPictureShape.java
+++ b/src/scratchpad/src/org/apache/poi/hslf/usermodel/HSLFPictureShape.java
@@ -146,13 +146,13 @@
LOG.atDebug().log("EscherContainerRecord.BSTORE_CONTAINER was not found ");
return null;
}
- List<EscherRecord> lst = bstore.getChildRecords();
+
int idx = getPictureIndex();
if (idx == 0){
LOG.atDebug().log("picture index was not found, returning ");
return null;
}
- return (EscherBSERecord)lst.get(idx-1);
+ return (EscherBSERecord) bstore.getChild(idx - 1);
}
/**
diff --git a/src/scratchpad/src/org/apache/poi/hslf/usermodel/HSLFSheet.java b/src/scratchpad/src/org/apache/poi/hslf/usermodel/HSLFSheet.java
index e241643..9a01066 100644
--- a/src/scratchpad/src/org/apache/poi/hslf/usermodel/HSLFSheet.java
+++ b/src/scratchpad/src/org/apache/poi/hslf/usermodel/HSLFSheet.java
@@ -236,10 +236,7 @@
return false;
}
- List<EscherRecord> lst = spgr.getChildRecords();
- boolean result = lst.remove(shape.getSpContainer());
- spgr.setChildRecords(lst);
- return result;
+ return spgr.removeChildRecord(shape.getSpContainer());
}
/**
diff --git a/src/scratchpad/src/org/apache/poi/hslf/usermodel/HSLFSlideShow.java b/src/scratchpad/src/org/apache/poi/hslf/usermodel/HSLFSlideShow.java
index 7066140..6dbb61a 100644
--- a/src/scratchpad/src/org/apache/poi/hslf/usermodel/HSLFSlideShow.java
+++ b/src/scratchpad/src/org/apache/poi/hslf/usermodel/HSLFSlideShow.java
@@ -1270,7 +1270,7 @@
record.setOffset(offset);
blipStore.addChildRecord(record);
- int count = blipStore.getChildRecords().size();
+ int count = blipStore.getChildCount();
blipStore.setOptions((short) ((count << 4) | 0xF));
return record;
diff --git a/src/scratchpad/src/org/apache/poi/hwpf/model/OfficeArtContent.java b/src/scratchpad/src/org/apache/poi/hwpf/model/OfficeArtContent.java
index 2ea08f5..35436bf 100644
--- a/src/scratchpad/src/org/apache/poi/hwpf/model/OfficeArtContent.java
+++ b/src/scratchpad/src/org/apache/poi/hwpf/model/OfficeArtContent.java
@@ -141,7 +141,7 @@
1);
for ( EscherContainerRecord dgContainer : getDgContainers() )
{
- for ( EscherRecord escherRecord : dgContainer.getChildRecords() )
+ for ( EscherRecord escherRecord : dgContainer )
{
if ( escherRecord.getRecordId() == (short) 0xF003 )
{
@@ -158,7 +158,7 @@
1);
for ( EscherContainerRecord spgrContainer : getSpgrContainers() )
{
- for ( EscherRecord escherRecord : spgrContainer.getChildRecords() )
+ for ( EscherRecord escherRecord : spgrContainer )
{
if ( escherRecord.getRecordId() == (short) 0xF004 )
{
diff --git a/src/scratchpad/src/org/apache/poi/hwpf/usermodel/OfficeDrawingsImpl.java b/src/scratchpad/src/org/apache/poi/hwpf/usermodel/OfficeDrawingsImpl.java
index 253cc07..ec54777 100644
--- a/src/scratchpad/src/org/apache/poi/hwpf/usermodel/OfficeDrawingsImpl.java
+++ b/src/scratchpad/src/org/apache/poi/hwpf/usermodel/OfficeDrawingsImpl.java
@@ -56,12 +56,10 @@
if (bContainer == null)
return null;
- final List<EscherRecord> bitmapRecords = bContainer.getChildRecords();
-
- if ( bitmapRecords.size() < bitmapIndex )
+ if ( bContainer.getChildCount() < bitmapIndex )
return null;
- EscherRecord imageRecord = bitmapRecords.get( bitmapIndex - 1 );
+ EscherRecord imageRecord = bContainer.getChild( bitmapIndex - 1 );
if ( imageRecord instanceof EscherBlipRecord )
{
diff --git a/src/scratchpad/testcases/org/apache/poi/hslf/usermodel/TestBackground.java b/src/scratchpad/testcases/org/apache/poi/hslf/usermodel/TestBackground.java
index c105e08..38bc03b 100644
--- a/src/scratchpad/testcases/org/apache/poi/hslf/usermodel/TestBackground.java
+++ b/src/scratchpad/testcases/org/apache/poi/hslf/usermodel/TestBackground.java
@@ -211,8 +211,7 @@
Document doc = ppt.getDocumentRecord();
EscherContainerRecord dggContainer = doc.getPPDrawingGroup().getDggContainer();
EscherContainerRecord bstore = HSLFShape.getEscherChild(dggContainer, EscherContainerRecord.BSTORE_CONTAINER);
- List<EscherRecord> lst = bstore.getChildRecords();
- return ((EscherBSERecord)lst.get(idx-1)).getRef();
+ return ((EscherBSERecord) bstore.getChild(idx - 1)).getRef();
}
return 0;
}
diff --git a/src/scratchpad/testcases/org/apache/poi/hslf/usermodel/TestPictures.java b/src/scratchpad/testcases/org/apache/poi/hslf/usermodel/TestPictures.java
index 40076c7..a196ed6 100644
--- a/src/scratchpad/testcases/org/apache/poi/hslf/usermodel/TestPictures.java
+++ b/src/scratchpad/testcases/org/apache/poi/hslf/usermodel/TestPictures.java
@@ -712,7 +712,7 @@
ByteArrayOutputStream inMemory = new ByteArrayOutputStream();
try (HSLFSlideShow ppt = HSLFTestDataSamples.getSlideShow("pictures.ppt")) {
originalOffsets = ppt.getPictureData().stream().mapToInt(HSLFPictureData::getOffset).toArray();
- originalNumberOfRecords = ppt.getPictureData().get(0).bStore.getChildRecords().size();
+ originalNumberOfRecords = ppt.getPictureData().get(0).bStore.getChildCount();
Random random = new Random();
for (HSLFPictureData picture : ppt.getPictureData()) {
@@ -729,7 +729,7 @@
assertArrayEquals(originalOffsets, offsets);
// Verify that there are the same number of records as in the original slideshow.
- int numberOfRecords = ppt.getPictureData().get(0).bStore.getChildRecords().size();
+ int numberOfRecords = ppt.getPictureData().get(0).bStore.getChildCount();
assertEquals(originalNumberOfRecords, numberOfRecords);
}
}
diff --git a/src/testcases/org/apache/poi/hssf/model/TestDrawingShapes.java b/src/testcases/org/apache/poi/hssf/model/TestDrawingShapes.java
index 673b548..22cd1ee 100644
--- a/src/testcases/org/apache/poi/hssf/model/TestDrawingShapes.java
+++ b/src/testcases/org/apache/poi/hssf/model/TestDrawingShapes.java
@@ -315,7 +315,7 @@
EscherContainerRecord spgrContainer =
agg1.getEscherContainer().getChildContainers().get(0);
// root spContainer + 2 spContainers for shapes
- assertEquals(3, spgrContainer.getChildRecords().size());
+ assertEquals(3, spgrContainer.getChildCount());
EscherSpRecord sp0 =
((EscherContainerRecord) spgrContainer.getChild(0)).getChildById(EscherSpRecord.RECORD_ID);
diff --git a/src/testcases/org/apache/poi/hssf/usermodel/TestHSSFComment.java b/src/testcases/org/apache/poi/hssf/usermodel/TestHSSFComment.java
index 0068f98..181cd50 100644
--- a/src/testcases/org/apache/poi/hssf/usermodel/TestHSSFComment.java
+++ b/src/testcases/org/apache/poi/hssf/usermodel/TestHSSFComment.java
@@ -213,7 +213,7 @@
HSSFCell cell = row.createCell(0);
cell.setCellComment(comment);
- assertEquals(comment.getEscherContainer().getChildRecords().size(), 5);
+ assertEquals(comment.getEscherContainer().getChildCount(), 5);
//sp record
byte[] expected = decompress("H4sIAAAAAAAAAFvEw/WBg4GBgZEFSHAxMAAA9gX7nhAAAAA=");
diff --git a/src/testcases/org/apache/poi/hssf/usermodel/TestPolygon.java b/src/testcases/org/apache/poi/hssf/usermodel/TestPolygon.java
index a144482..759e790 100644
--- a/src/testcases/org/apache/poi/hssf/usermodel/TestPolygon.java
+++ b/src/testcases/org/apache/poi/hssf/usermodel/TestPolygon.java
@@ -43,7 +43,7 @@
polygon.setPoints( new int[]{0, 90, 50}, new int[]{5, 5, 44} );
polygon.setShapeId(1024);
- assertEquals(polygon.getEscherContainer().getChildRecords().size(), 4);
+ assertEquals(polygon.getEscherContainer().getChildCount(), 4);
//sp record
byte[] expected = decompress("H4sIAAAAAAAAAGNi4PrAwQAELEDMxcAAAAU6ZlwQAAAA");
diff --git a/src/testcases/org/apache/poi/hssf/usermodel/TestText.java b/src/testcases/org/apache/poi/hssf/usermodel/TestText.java
index f00bf02..a7065cd 100644
--- a/src/testcases/org/apache/poi/hssf/usermodel/TestText.java
+++ b/src/testcases/org/apache/poi/hssf/usermodel/TestText.java
@@ -41,7 +41,7 @@
HSSFPatriarch patriarch = sh.createDrawingPatriarch();
HSSFTextbox textbox = patriarch.createTextbox(new HSSFClientAnchor());
- assertEquals(textbox.getEscherContainer().getChildRecords().size(), 5);
+ assertEquals(textbox.getEscherContainer().getChildCount(), 5);
//sp record
byte[] expected = decompress("H4sIAAAAAAAAAFvEw/WBg4GBgZEFSHAxMAAA9gX7nhAAAAA=");
