Index: modules/luni/src/main/java/java/util/ArrayList.java =================================================================== --- modules/luni/src/main/java/java/util/ArrayList.java (revision 981664) +++ modules/luni/src/main/java/java/util/ArrayList.java (working copy) @@ -69,7 +69,7 @@ /** * Constructs a new instance of {@code ArrayList} containing the elements of * the specified collection. The initial size of the {@code ArrayList} will - * be 10% higher than the size of the specified collection. + * be 10% larger than the size of the specified collection. * * @param collection * the collection of elements to add. @@ -78,6 +78,9 @@ firstIndex = 0; Object[] objects = collection.toArray(); int size = objects.length; + + // REVIEW: Created 2 array copies of the original collection here + // Could it be better to use the collection iterator and copy once? array = newElementArray(size + (size / 10)); System.arraycopy(objects, 0, array, 0, size); lastIndex = size; @@ -110,6 +113,7 @@ growForInsert(location, 1); } else if ((location < size / 2 && firstIndex > 0) || lastIndex == array.length) { + // REVIEW: Why not evaluate (lastIndex==array.length) first to save the divide? System.arraycopy(array, firstIndex, array, --firstIndex, location); } else { @@ -120,11 +124,13 @@ } array[location + firstIndex] = object; } else if (location == 0) { + // REVIEW: Does growAtFront() check the end to see if shifting the array is possible? Same for growAtEnd(). if (firstIndex == 0) { growAtFront(1); } array[--firstIndex] = object; } else if (location == size) { + // REVIEW: Why not just call add()? Matching RI behaviour? if (lastIndex == array.length) { growAtEnd(1); } @@ -174,6 +180,7 @@ @Override public boolean addAll(int location, Collection collection) { int size = lastIndex - firstIndex; + // REVIEW: Inconsistent exception case with add(location,object) method if (location < 0 || location > size) { throw new IndexOutOfBoundsException( // luni.0A=Index: {0}, Size: {1} @@ -182,20 +189,28 @@ Integer.valueOf(lastIndex - firstIndex))); } if (this == collection) { + // REVIEW: Do we need this clone if we just use toArray on the next line? collection = (ArrayList)clone(); } Object[] dumparray = collection.toArray(); int growSize = dumparray.length; + // REVIEW: Why do this check here rather than check collection.size() earlier? RI behaviour? if (growSize == 0) { return false; } if (0 < location && location < size) { + // REVIEW: we use array.length - growSize in 3 places - precalculate... if (array.length - size < growSize) { + // REVIEW: why grow growSize? Does growForInsert() + // check we don't allocate too much? growForInsert(location, growSize); } else if ((location < size / 2 && firstIndex > 0) || lastIndex > array.length - growSize) { + // REVIEW: If condition above could be switched so divide is done 2nd, same as add() int newFirst = firstIndex - growSize; + // REVIEW: Have we optimised for the case where there is enough space at the + // end for growSize rather than doing 2 array copies? if (newFirst < 0) { int index = location + firstIndex; System.arraycopy(array, index, array, index - newFirst, @@ -215,6 +230,7 @@ growAtFront(growSize); firstIndex -= growSize; } else if (location == size) { + // REVIEW: Don't need the above check as it can be no other case. Make it just an else. if (lastIndex > array.length - growSize) { growAtEnd(growSize); } @@ -259,7 +275,10 @@ @Override public void clear() { if (firstIndex != lastIndex) { + // REVIEW: Should we use Arrays.fill() instead of just allocating a new array? + // Should we use the same sized array? Arrays.fill(array, firstIndex, lastIndex, null); + // REVIEW: Should the indexes point into the middle of the array rather than 0? firstIndex = lastIndex = 0; modCount++; } @@ -319,6 +338,7 @@ */ public void ensureCapacity(int minimumCapacity) { if (array.length < minimumCapacity) { + // REVIEW: Why do we check the firstIndex first? Growing the end makes more sense if (firstIndex > 0) { growAtFront(minimumCapacity - array.length); } else { @@ -329,6 +349,7 @@ @Override public E get(int location) { + // REVIEW: Throw exception first, then return? if (0 <= location && location < (lastIndex - firstIndex)) { return array[firstIndex + location]; } @@ -342,7 +363,9 @@ private void growAtEnd(int required) { int size = lastIndex - firstIndex; if (firstIndex >= required - (array.length - lastIndex)) { + // REVIEW: Should use size! We don't seem to need newLast - just use size calculated above int newLast = lastIndex - firstIndex; + // REVIEW: Why not just a !=0 check here? And size cannot be 0 unless required is 0, which does not happen if (size > 0) { System.arraycopy(array, firstIndex, array, 0, size); int start = newLast < firstIndex ? firstIndex : newLast; @@ -351,6 +374,8 @@ firstIndex = 0; lastIndex = newLast; } else { + // REVIEW: If size is 0? + // Does size/2 seems a little high! int increment = size / 2; if (required > increment) { increment = required; @@ -359,6 +384,8 @@ increment = 12; } E[] newArray = newElementArray(size + increment); + // REVIEW: Just check size !=0? same as earlier check - + // can be pulled out to earlier in the method? if (size > 0) { System.arraycopy(array, firstIndex, newArray, 0, size); firstIndex = 0; @@ -370,10 +397,13 @@ private void growAtFront(int required) { int size = lastIndex - firstIndex; + // REVIEW: replace lastIndex - firstIndex with size ... in all methods that have it if (array.length - lastIndex + firstIndex >= required) { int newFirst = array.length - size; + // REVIEW: as growAtEnd, why not move size == 0 out as special case if (size > 0) { System.arraycopy(array, firstIndex, array, newFirst, size); + // REVIEW: firstIndex + size == lastIndex ?? int length = firstIndex + size > newFirst ? newFirst : firstIndex + size; Arrays.fill(array, firstIndex, length, null); @@ -400,6 +430,9 @@ } private void growForInsert(int location, int required) { + // REVIEW: we grow too quickly because we are called with the size of the new + // collection to add without taking in to account the free space we + // already have int size = lastIndex - firstIndex; int increment = size / 2; if (required > increment) { @@ -409,6 +442,8 @@ increment = 12; } E[] newArray = newElementArray(size + increment); + // REVIEW: biased towards leaving space at the beginning? + // perhaps newFirst should be (increment-required)/2? int newFirst = increment - required; // Copy elements after location to the new array skipping inserted // elements @@ -424,6 +459,7 @@ @Override public int indexOf(Object object) { + // REVIEW: should contains call this method? if (object != null) { for (int i = firstIndex; i < lastIndex; i++) { if (object.equals(array[i])) { @@ -475,12 +511,14 @@ @Override public E remove(int location) { E result; + // REVIEW: move exception to the beginning int size = lastIndex - firstIndex; if (0 <= location && location < size) { if (location == size - 1) { result = array[--lastIndex]; array[lastIndex] = null; } else if (location == 0) { + // REVIEW: this if test is simpler why isn't it first? result = array[firstIndex]; array[firstIndex++] = null; } else { @@ -496,6 +534,8 @@ array[--lastIndex] = null; } } + // REVIEW: we can move this to the first if case since it + // can only occur when size==1 if (firstIndex == lastIndex) { firstIndex = lastIndex = 0; } @@ -534,11 +574,13 @@ */ @Override protected void removeRange(int start, int end) { + // REVIEW: does RI call this from remove(location) + // REVIEW: put exception case at the beginning if (start >= 0 && start <= end && end <= (lastIndex - firstIndex)) { if (start == end) { return; } - int size = lastIndex - firstIndex; + int size = lastIndex - firstIndex; // REVIEW: move this up so we can use it above if (end == size) { Arrays.fill(array, firstIndex + start, lastIndex, null); lastIndex = firstIndex + start; @@ -546,6 +588,7 @@ Arrays.fill(array, firstIndex, firstIndex + end, null); firstIndex += end; } else { + // REVIEW: should this optimize to do the smallest copy? System.arraycopy(array, firstIndex + end, array, firstIndex + start, size - end); int newLast = lastIndex + start - end; @@ -554,6 +597,8 @@ } modCount++; } else { + // REVIEW: message should indicate which index is out of range + // REVIEW: end might be in range so reporting based on that is bogus throw new IndexOutOfBoundsException( // luni.0B=Array index out of range: {0} Messages.getString("luni.0B", //$NON-NLS-1$ @@ -575,6 +620,7 @@ */ @Override public E set(int location, E object) { + // REVIEW: move the exception to beginning if (0 <= location && location < (lastIndex - firstIndex)) { E result = array[firstIndex + location]; array[firstIndex + location] = object; @@ -594,6 +640,7 @@ */ @Override public int size() { + // REVIEW: We should track firstIndex and size rather than lastIndex return lastIndex - firstIndex; } @@ -636,6 +683,7 @@ } System.arraycopy(array, firstIndex, contents, 0, size); if (size < contents.length) { + // REVIEW: do we use this incorrectly - i.e. do we null the rest out? contents[size] = null; } return contents;