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

17 files changed, 29 insertions, 34 deletions

diff --git a/src/java/org/apache/poi/ddf/EscherContainerRecord.java b/src/java/org/apache/poi/ddf/EscherContainerRecord.java
index 474b24a436..bbe7efac65 100644
--- a/src/java/org/apache/poi/ddf/EscherContainerRecord.java
+++ b/src/java/org/apache/poi/ddf/EscherContainerRecord.java
@@ -158,6 +158,10 @@ public final class EscherContainerRecord extends EscherRecord implements Iterabl
         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 b6d60ec471..c5703dfca3 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 @@ public final class InternalWorkbook {
 
         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 92d66e1db6..964356f5a6 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 @@ public abstract class HSSFShape implements Shape {
                 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 @@ public abstract class HSSFShape implements Shape {
                 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 4652693837..b8001fc651 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 @@ public class HSSFShapeGroup extends HSSFShape implements HSSFShapeContainer {
         // 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 8463080ca3..cff59a70c5 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 @@ public final class SlideShowRecordDumper {
         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 5275669b76..bb2c52b214 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 @@ public final class HSLFFill {
             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 @@ public final class HSLFFill {
         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 548fdff3d1..6aba4b23ff 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 @@ public class HSLFPictureShape extends HSLFSimpleShape implements PictureShape<HS
             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 e241643869..9a010661c5 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 @@ public abstract class HSLFSheet implements HSLFShapeContainer, Sheet<HSLFShape,H
             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 7066140bd6..6dbb61a0ba 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 @@ public final class HSLFSlideShow extends POIDocument implements SlideShow<HSLFSh
 		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 2ea08f5ed1..35436bf75a 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 @@ public final class OfficeArtContent {
                 1);
         for ( EscherContainerRecord dgContainer : getDgContainers() )
         {
-            for ( EscherRecord escherRecord : dgContainer.getChildRecords() )
+            for ( EscherRecord escherRecord : dgContainer )
             {
                 if ( escherRecord.getRecordId() == (short) 0xF003 )
                 {
@@ -158,7 +158,7 @@ public final class OfficeArtContent {
                 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 253cc07536..ec5477725a 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 @@ public class OfficeDrawingsImpl implements OfficeDrawings
         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 c105e08518..38bc03bb28 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 @@ public final class TestBackground {
             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 40076c758f..a196ed685b 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 @@ public final class TestPictures {
         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 @@ public final class TestPictures {
             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 673b548e38..22cd1ee11c 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 @@ class TestDrawingShapes {
         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 0068f98305..181cd5052b 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 @@ final class TestHSSFComment extends BaseTestCellComment {
         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 a144482e88..759e790a6f 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 @@ class TestPolygon {
         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 f00bf02dfe..a7065cda2e 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 @@ class TestText {
         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=");