Refactor KllLongsSketch classes

[Abhiram98]

Refactor KllDirectLongsSketch, KllHeapLongsSketch, KllHeapLongsSketch by primarily renaming variables, splitting long methods.

List of changes:

KllDirectLongsSketch.java

• rename "wmem" -> "writableMemory"
• rename "memReqSvr" -> "memoryRequestServer"

KllHeapLongsSketch.java

• extract "initializeSketchFromMemory" method
• convert if statement to switch statement inside "initializeSketchFromMemory"
• convert if statement to ternary operator in method "getItemAsString"

KllLongsHelper.java

• rename "lngSk" to "longSketch"
• rename "workbuf" to "workBuffer"
• rename "ub" to "upperBound"
• extract "updateMinMaxItems" method
• rename "inBuf" to "inputBuffer"
• rename "rawBeg" to "rawBegin"
• extract "populateRandomlyHalveDownLongs" method
• extract "handleArrayCopy" method
• extract "populateMergeSortedLongArrays" method

[leerho]

I want to thank you for your interest in our Library and wanting to make a contribution.

However, I have some serious concerns about this particular set of changes.

• Renaming of many of these variables to their longer, more verbose form is a stylistic change, and out-of-step with the use of some of these variables throughout the entire library.

  • "mem" for "memory" and "wmem" for "writableMemory" are used throughout the library and easy to remember. It is not like they were obscure names like "x" and "y".
  • Changing "workBuf" to "workBuffer" is hardly an improvement.
  • Your stylistic changes are also inconsistent. You changed "inBuf" to "inputBuffer" but didn't change "outBuf" to "outputBuffer".
  • Your changes also are not consistent even within the KLL classes also used by Kll*Longs*, such as KllHelper, KllSketch, etc. And, of course, changing these variable names in these classes would make them inconsistent with all the other KLL doubles, floats, and items classes.

• Extracting out methods that are only used once is quite marginal in its usefulness.

• But a more serious impact of these changes is that the code no longer aligns with the code of the parallel classes for "doubles" and "floats" and mostly aligns with "items". These classes were carefully constructed so that one could examine the code for KllHeapLongsSketch and KllHeapDoublesSketch side by side with a comparison editor and visually confirm that the code is identical for both classes, except for the naming of the type variables, where it is, say, "long" in one class and "double" in the other. These classes align line-for-line, which makes debugging and editing much simpler. A change in one KLL type must also be changed in the others. Your changes destroyed this ease of editing, checking and debugging.

• Java doesn't have an efficient built-in template capability, like C++, where this kind of duplication of code is not necessary. Java does have generics, but they are very inefficient for this kind of code. The only other alternative is to use a template pre-processor, which I have seriously considered (Java uses template pre-processors internally), but have not yet implemented.

Change for change-sake is not a contribution, and stylistic changes such as these are actually a detriment because they are not consistent with the rest of the library. And in this case, actually destroyed the ease of checking and debugging across parallel classes.

When making contributions, it is important to be sensitive to the style of the original authors, even if it is not your preferred style. And in a library, such as this, it is important to be aware of how your proposed changes might impact the rest of the library.

I am closing this PR.

[Abhiram98]

Thank you for your detailed response. Your feedback is very valuable to me!