From 68168991067328b0944fcde2a61ab47f797817ec Mon Sep 17 00:00:00 2001
From: Nick Burch <nick@apache.org>
Date: Tue, 25 Oct 2016 09:33:37 +0000
Subject: [PATCH] Refactor child pointer logic to the factory

git-svn-id: https://svn.apache.org/repos/asf/poi/trunk@1766492 13f79535-47bb-0310-9956-ffa450edef68
---
 .../org/apache/poi/hdgf/pointers/Pointer.java |  4 ++
 .../poi/hdgf/pointers/PointerFactory.java     | 53 ++++++++-----------
 .../apache/poi/hdgf/pointers/PointerV5.java   | 31 +++++++++++
 .../apache/poi/hdgf/pointers/PointerV6.java   | 32 +++++++++++
 .../hdgf/streams/PointerContainingStream.java | 34 +-----------
 .../apache/poi/hdgf/streams/StreamTest.java   | 14 +++++
 6 files changed, 104 insertions(+), 64 deletions(-)

diff --git a/src/scratchpad/src/org/apache/poi/hdgf/pointers/Pointer.java b/src/scratchpad/src/org/apache/poi/hdgf/pointers/Pointer.java
index d10f154e2..a2c5854f7 100644
--- a/src/scratchpad/src/org/apache/poi/hdgf/pointers/Pointer.java
+++ b/src/scratchpad/src/org/apache/poi/hdgf/pointers/Pointer.java
@@ -45,6 +45,10 @@ public abstract class Pointer {
 	}
 
 	public abstract int getSizeInBytes();
+	public abstract int getNumPointersOffset(byte[] data);
+	public abstract int getNumPointers(int offset, byte[] data);
+	public abstract int getPostNumPointersSkip();
+	
 	public abstract boolean destinationHasStrings();
 	public abstract boolean destinationHasPointers();
 	public abstract boolean destinationHasChunks();
diff --git a/src/scratchpad/src/org/apache/poi/hdgf/pointers/PointerFactory.java b/src/scratchpad/src/org/apache/poi/hdgf/pointers/PointerFactory.java
index 640396cf5..8506c83a9 100644
--- a/src/scratchpad/src/org/apache/poi/hdgf/pointers/PointerFactory.java
+++ b/src/scratchpad/src/org/apache/poi/hdgf/pointers/PointerFactory.java
@@ -31,6 +31,9 @@ public final class PointerFactory {
 	}
 	public int getVersion() { return version; }
 
+	/**
+	 * Creates a single Pointer from the data at the given offset
+	 */
 	public Pointer createPointer(byte[] data, int offset) {
 		Pointer p;
 		if(version >= 6) {
@@ -57,39 +60,25 @@ public final class PointerFactory {
 	}
 	
 	/**
-	 * In a {@link PointerContainingStream}, where would the
-	 *  number of child pointers be stored for this kind of Pointer?
+	 * Parsers the {@link PointerContainingStream} contents and
+	 *  creates all the child Pointers for it
 	 */
-	public int identifyNumPointersOffset(Pointer pointer, byte[] data) {
-	    if (pointer instanceof PointerV6) {
-	        // V6 stores it as the first value in the stream
-	        return (int)LittleEndian.getUInt(data, 0);
-	    } else if (pointer instanceof PointerV5) {
-	        // V5 uses fixed offsets
-	        switch (pointer.type) {
-    	         case 0x1d:
-    	         case 0x4e:
-    	            return 0x24-6;
-    	         case 0x1e:
-    	            return 0x3c-6;
-                 case 0x14:
-                     return 0x88-6;
-	        }
-	        return 10;
-	    } else {
-            throw new IllegalArgumentException("Unsupported Pointer type " + pointer);
-	    }
-	}
-	
-	public int identifyNumPointers(Pointer pointer, int offset, byte[] data) {
-        if (pointer instanceof PointerV6) {
-            // V6 stores it a 32 bit number at the offset
-            return (int)LittleEndian.getUInt(data, offset);
-        } else if (pointer instanceof PointerV5) {
-            // V5 stores it as a 16 bit number at the offset
-            return LittleEndian.getShort(data, offset);
-        } else {
-            throw new IllegalArgumentException("Unsupported Pointer type " + pointer);
+	public Pointer[] createContainerPointers(Pointer parent, byte[] data) {
+	    // Where in the stream does the "number of pointers" offset live?
+	    int numPointersOffset = parent.getNumPointersOffset(data);
+	    // How many do we have?
+	    int numPointers = parent.getNumPointers(numPointersOffset, data);
+	    // How much to skip for the num pointers + any extra data?
+	    int skip = parent.getPostNumPointersSkip();
+	    
+	    // Create
+	    int pos = numPointersOffset + skip;
+	    Pointer[] childPointers = new Pointer[numPointers];
+        for(int i=0; i<numPointers; i++) {
+            childPointers[i] = this.createPointer(data, pos);
+            pos += childPointers[i].getSizeInBytes();
         }
+        
+        return childPointers;
 	}
 }
diff --git a/src/scratchpad/src/org/apache/poi/hdgf/pointers/PointerV5.java b/src/scratchpad/src/org/apache/poi/hdgf/pointers/PointerV5.java
index 23c2a6c47..2fde9a2d5 100644
--- a/src/scratchpad/src/org/apache/poi/hdgf/pointers/PointerV5.java
+++ b/src/scratchpad/src/org/apache/poi/hdgf/pointers/PointerV5.java
@@ -17,6 +17,8 @@
 
 package org.apache.poi.hdgf.pointers;
 
+import org.apache.poi.util.LittleEndian;
+
 /**
  * A Pointer from v5
  */
@@ -43,4 +45,33 @@ public final class PointerV5 extends Pointer {
 	 * With v6 pointers, the on-disk size is 16 bytes
 	 */
 	public int getSizeInBytes() { return 16; }
+	
+	/**
+	 * Depends on the type only, not stored
+	 */
+    public int getNumPointersOffset(byte[] data) {
+        switch (type) {
+            case 0x1d:
+            case 0x4e:
+               return 0x24-6;
+            case 0x1e:
+               return 0x3c-6;
+            case 0x14:
+                return 0x88-6;
+       }
+       return 10;
+    }
+    /**
+     * 16 bit int at the given offset
+     */
+    public int getNumPointers(int offset, byte[] data) {
+        // V5 stores it as a 16 bit number at the offset
+        return LittleEndian.getShort(data, offset);
+    }
+    /**
+     * Just the 2 bytes of the number of pointers
+     */
+    public int getPostNumPointersSkip() {
+        return 2;
+    }
 }
diff --git a/src/scratchpad/src/org/apache/poi/hdgf/pointers/PointerV6.java b/src/scratchpad/src/org/apache/poi/hdgf/pointers/PointerV6.java
index 75445dcd1..09b00fb4b 100644
--- a/src/scratchpad/src/org/apache/poi/hdgf/pointers/PointerV6.java
+++ b/src/scratchpad/src/org/apache/poi/hdgf/pointers/PointerV6.java
@@ -17,6 +17,8 @@
 
 package org.apache.poi.hdgf.pointers;
 
+import org.apache.poi.util.LittleEndian;
+
 /**
  * A Pointer from v6+
  */
@@ -42,4 +44,34 @@ public final class PointerV6 extends Pointer {
 	 * With v6 pointers, the on-disk size is 18 bytes
 	 */
 	public int getSizeInBytes() { return 18; }
+	
+	/**
+	 * Stored within the data
+	 */
+    public int getNumPointersOffset(byte[] data) {
+        return getNumPointersOffsetV6(data);
+    }
+    public static int getNumPointersOffsetV6(byte[] data) {    
+        // V6 stores it as the first value in the stream
+        return (int)LittleEndian.getUInt(data, 0);
+    }
+    /**
+     * 32 bit int at the given offset
+     */
+    public int getNumPointers(int offset, byte[] data) {
+        return getNumPointersV6(offset, data);
+    }
+    public static int getNumPointersV6(int offset, byte[] data) {
+        // V6 stores it a 32 bit number at the offset
+        return (int)LittleEndian.getUInt(data, offset);
+    }
+    /**
+     * 4 bytes of the number, and 4 more unknown bytes
+     */
+    public int getPostNumPointersSkip() {
+        return getPostNumPointersSkipV6();
+    }
+    public static int getPostNumPointersSkipV6() {
+        return 8;
+    }
 }
diff --git a/src/scratchpad/src/org/apache/poi/hdgf/streams/PointerContainingStream.java b/src/scratchpad/src/org/apache/poi/hdgf/streams/PointerContainingStream.java
index d07aa0450..e7f3cf39d 100644
--- a/src/scratchpad/src/org/apache/poi/hdgf/streams/PointerContainingStream.java
+++ b/src/scratchpad/src/org/apache/poi/hdgf/streams/PointerContainingStream.java
@@ -20,8 +20,6 @@ package org.apache.poi.hdgf.streams;
 import org.apache.poi.hdgf.chunks.ChunkFactory;
 import org.apache.poi.hdgf.pointers.Pointer;
 import org.apache.poi.hdgf.pointers.PointerFactory;
-import org.apache.poi.hdgf.pointers.PointerV6;
-import org.apache.poi.util.LittleEndian;
 
 /**
  * A stream that holds pointers, possibly in addition to some
@@ -33,42 +31,14 @@ public class PointerContainingStream extends Stream { // TODO - instantiable sup
 
 	private ChunkFactory chunkFactory;
 	private PointerFactory pointerFactory;
-	private int numPointersLocalOffset;
 
 	protected PointerContainingStream(Pointer pointer, StreamStore store, ChunkFactory chunkFactory, PointerFactory pointerFactory) {
 		super(pointer, store);
 		this.chunkFactory = chunkFactory;
 		this.pointerFactory = pointerFactory;
-
-		// Find the offset to the number of child pointers we have
-		numPointersLocalOffset = pointerFactory.identifyNumPointersOffset(
-				pointer, store.getContents()
-		);
-
-		// Generate the objects for the pointers we contain
-		int numPointers = pointerFactory.identifyNumPointers(
-				pointer, numPointersLocalOffset, store.getContents()
-		);
-		childPointers = new Pointer[numPointers];
 		
-		// Prepare to read the children
-        int pos = numPointersLocalOffset;
-
-        // v5 only has another (unknown) 4 byte value
-        // TODO Should this logic go into PointerFactory?
-        if (pointer instanceof PointerV6) {
-            pos += 4 + 4;
-        } else {
-            pos += 2;
-        }
-
-		// Now create the pointer objects
-		for(int i=0; i<numPointers; i++) {
-			childPointers[i] = pointerFactory.createPointer(
-					store.getContents(), pos
-			);
-			pos += childPointers[i].getSizeInBytes();
-		}
+		// Have the child pointers identified and created
+		childPointers = pointerFactory.createContainerPointers(pointer, store.getContents());
 	}
 
 	/**
diff --git a/src/scratchpad/testcases/org/apache/poi/hdgf/streams/StreamTest.java b/src/scratchpad/testcases/org/apache/poi/hdgf/streams/StreamTest.java
index 99bb4a2bc..aef1782fa 100644
--- a/src/scratchpad/testcases/org/apache/poi/hdgf/streams/StreamTest.java
+++ b/src/scratchpad/testcases/org/apache/poi/hdgf/streams/StreamTest.java
@@ -18,6 +18,7 @@
 package org.apache.poi.hdgf.streams;
 
 import org.apache.poi.hdgf.pointers.Pointer;
+import static org.apache.poi.hdgf.pointers.PointerV6.*;
 
 import junit.framework.TestCase;
 
@@ -41,7 +42,20 @@ public abstract class StreamTest extends TestCase {
         public boolean destinationHasPointers() { return hasPointers; }
 		@Override
         public boolean destinationHasStrings() { return false; }
+		
 		@Override
         public int getSizeInBytes() { return -1; }
+		@Override
+        public int getNumPointersOffset(byte[] data) { 
+		    return getNumPointersOffsetV6(data); 
+		}
+        @Override
+        public int getNumPointers(int offset, byte[] data) { 
+            return getNumPointersV6(offset, data);
+        }
+        @Override
+        public int getPostNumPointersSkip() {
+            return getPostNumPointersSkipV6();
+        }
 	}
 }