Skip to main content
added 6 characters in body
Source Link

This is a badly named method. It is the byte / digit value that is being calculatedextracted. That it is stored in the also badly named "map" in inconsequential for the method itself.

This is a badly named method. It is the digit value that is being calculated. That it is stored in the also badly named "map" in inconsequential for the method itself.

This is a badly named method. It is the byte / digit value that is being extracted. That it is stored in the also badly named "map" in inconsequential for the method itself.

Source Link

In general the code uses the right spacing and variable naming.


 * This class provides the method for sorting {@code int} arrays using 
 * least-significant digit (LSD) radix sort.

No it doesn't, unless you call a byte a digit; this should be mentioned at least to future developers that need to look at the code (this might be less important to users as the functionality should remain the same even if digits are used).

I'd also indicate that this is a helper or utility class, to save users from looking for constructors or factory methods.

Possibly it would also be a good idea to indicate the order of operations required for a sort. That's the only reason to prefer it over other options it seems to me.


LSDRadixsort

It depends, but nowadays I see often that the acronyms are also in CamelCase: LsdRadixSort. LSDRadixsort is fine of course but I see the point of the proponents of using smaller caps for anything but the first letter.


private static final int NUMBER_OF_COUNTERS = 256;

To me this was just the size of a buffer, but it it seems it is the number of values within a byte. In that case you could use BYTE_VALUES or something similar. Or assign it Byte.MAX_VALUE - Byte.MIN_VALUE + 1 to indicate the bytes.


checkRangeIndices(array.length,
                      fromIndex,
                      toIndex);

The disadvantage of such a check method is that the exception appears to be coming from another method than the one requiring the parameters. This may obscure the error somewhat.

There are ways to alter the stack trace, but they are rather ugly so I won't go into them.


// Trivially sorted:


// Sort first by least-significant bytes, then by second 
// least-significant, and finally by third least-signficant byte:

To me the use of capitalization and the colon is somewhat annoying and unnecessary.


    int[] counterMap = new int[NUMBER_OF_COUNTERS];

A map is generally not an array, here I would have expected a comment about how the map is being used.


    // Spawn sorting:

Generally we associate spawning with the spawning of a process or a thread. I would not use any comment just to indicate a call.


for (int byteIndex = 0; byteIndex != 3; byteIndex++) {

Generally byteIndex < 3 is used, and I would keep to that if simply to avoid confusion. I'm seeing this at multiple places in the code.


extractCounterIndex

This is a badly named method. It is the digit value that is being calculated. That it is stored in the also badly named "map" in inconsequential for the method itself.


buffer[counterMap[index]-- - 1] = array[i];

All those comments, and this hard to read piece of code that performs the magic is not explained.


There is a lot of code which is duplicated just to get around the issue of a signed byte. It should be possible to refactor that and get rid of the spurious code (mapping from [-128, -127] to [0, 255] and back again when required). As it is, if there was a bug or other fix then it might end up in one method and not in the other.

Maybe it would be an interesting idea to also be able to treat integers as unsigned.