Return to Answer

• 1

• 113k
• 15
• 154
• 396

• 98.1k
• 17
• 219
• 419

Nice improvement. There is much less to review now, which is a good thing. Similarly, because there is less, you can also see things that the previous review missed.

##Naming

While Arrays is a good name for this class, it is too good. It conflicts with java.util.Arrays and it makes sense to name it differently so people can conveniently use both the java.util and your version of Arrays at the same time.

##Equals

Consider this code here which is from the areEquals(...) method:

 for (int i = 0; i < arrays[0].length; ++i) {
 for (int j = 0; j < arrays.length - 1; ++j) {
 if (arrays[j][i] != arrays[j + 1][i]) {
 return false;
 }
 }
 }

The code compares each member of each array with the members in the 'next' array. This should be using equals rather than identity == compares. Additionally, to handle null values in the array, it would make sense to use the Objects.equals(..., ...) method from java.util`. The inner condition would be:

if (!Objects.equals(arrays[j][i], arrays[j+1][i])) {
 return false;
}

As a side note, one possible optimization would be to calculate a hash code for the entire arrays before comparing them. This makes sense if you expect there to be differences more often than you expect them to be equals. If the hashCodes are different then they can never be equals:

int targetHash = java.util.Arrays.hashCode(arrays[j]);

then compare that to the hashCodes of the other arrays. Only if they are all the same do you need to do the actual comparisons.

Again, this only makes sense if you expect there to be differences more often than not.

##Sorting

In sortImpl I would really prefer if the contents of the merge sort block were put in to a separate method. Having parts of two sorts in one method is... messy. in this case, the clean=up swap after the merge (if needed), is messy. I would extract that all in to a method that takes the recursion depth as an input as well, and calls the mergeSort and does the cleanUp. This way the radix-sort method becomes:

if (toIndex - fromIndex <= MERGESORT_THRESHOLD) {
 mergeSortAndClean(...., recursionDepth);
 return;
}

The documentation here is lacking:

 if (recursionDepth == 7) {
 // There is nowhere to recur, return.
 return;
 }

We should have a reminder why the 7 is 'magic', or it should be a proper constant. I can't really remember the significance..... note that the JavaDoc on your method still refers to byteIndex, and should be for the recursionDepth instead....

Also, I believe the recursionDepth == 7 will never happen because the mergesort threshold will happen first, right?

All in all, it looks good though. Nicely done. I hope it makes more sense to you now too.