What to do with "internal implementation detail" classes and other "utils" classes?

[jbduncan]

@jrtom Currently, there are a number of classes in JUNG which seem to be either internal implementation details which nonetheless have the public modifier, meaning JUNG users can use them as well; or which look like they were designed specifically for JUNG but may be useful in a more general sense and could be part of the "endorsed" public API.

Two classes come to mind:

1. edu.uci.ics.jung.algorithms.util.BasicMapEntry: it seems that this class is just a simple implementation of Map.Entry and is only used by DijkstraDistance, so it looks to be just an implementation detail, despite it being a public type. In this case, it could simply be replaced with AbstractMap.SimpleImmutableEntry or Guava's Maps.immutableEntry().
2. edu.uci.ics.jung.algorithms.util.MaxBinaryHeap: this seems like a potentially generally useful collection class. Might it be worth moving into its own separate package e.g. edu.uci.ics.jung.collect, and perhaps into its own module (jung-collect)?

There may very well be other classes like these two that deserve this sort of attention as well.

One possible way of treating such classes, if we still find them useful, would be to move them into appropriate internal subpackages, and state in the README.md or elsewhere that all classes in subpackages with the name internal are implementation details and may go away in future releases. This would reduce the likelihood of people depending on those classes, giving us a bit more room to organise the code base better.

[jrtom]

These are good questions. :)

For BasicMapEntry, I think that the appropriate answer is to get rid of that class entirely. Options for doing so include (a) using Maps.immutableEntry(), (b) refactoring the code a bit to separate extracting the distance for a node from updating the internal data structure, or (c) using @AutoValue to define a value type specific to this purpose. I'm leaning a bit toward (c), but in any event this aspect of the implementation should be an internal one that we can feel free to swap out.

Regarding MaxBinaryHeap, I definitely don't want to create a new module for such things, although a new package might not be inappropriate. I'd rather try to see whether I can incorporate something like it into Guava, though.

[jbduncan]

Hi @jrtom!

For BasicMapEntry, I think that AutoValue is probably overkill, considering we can easily use AbstractMap.SimpleImmutableEntry or Maps.immutableEntry() in this case.

Would MaxBinaryHeap be worth moving to a edu.uci.ics.jung.algorithms.internal subpackage for now?

How about other classes in **/util/** subpackages that cannot be made non-public or removed? Should those go into **/internal/** subpackages as well?

Do you have any particular thoughts on whether we should document that classes in **/util/** or **/internal/** are implementation details and may be removed without warning in future releases?