search

jrtom / jung

JUNG: Java Universal Network/Graph Framework
Other
403 stars 115 forks source link

Consider renaming `TreeUtils` and/or `Graphs` #166

Open jbduncan opened 6 years ago

jbduncan commented 6 years ago

As I was working on tests for https://github.com/jrtom/jung/issues/83, I noticed first of all that TreeUtils and Graphs, both being utility classes, have inconsistent names, and secondly that Graphs's name conflicts with Guava's Graphs class.

AFAICT, we have a few options:

  1. Rename TreeUtils to Trees and Graphs to MoreGraphs.

    Pros: a. TreeUtils and Graphs would be more self-consistent, reducing surprise for new users. b. Guava naming conventions would be followed, reducing surprise for those already using Guava. c. Users wouldn't be forced to write edu.uci.ics.jung.graph.util.Graphs or com.google.common.graph.Graphs to use both Graphs classes in the same source file.

    Cons: a. Existing users may be surprised by this change. b. It would be backwards incompatible.

  2. Rename Graphs to GraphUtils.

    Pros: a. TreeUtils and Graphs would be more self-consistent, reducing surprise for new users. b. Users wouldn't be forced to write edu.uci.ics.jung.graph.util.Graphs or com.google.common.graph.Graphs to use both Graphs classes in the same source file.

    Cons: a. Existing users may be surprised by this change. b. It would be backwards incompatible. c. Users coming over from Guava would be surprised by the use of "Utils" instead of "s" for utility classes pertaining to graph data structures.

  3. Do nothing. Pro: a. No extra changes on our part.

    Con: b. New users and users coming from Guava would be surprised by the naming inconsistency.

IMO, option (1) is the best, as it aligns our naming conventions closer to Guava's, and I consider JUNG 3.0 to be an extension to com.google.common.graph.

@jrtom @tomnelson WDYT? I'm happy to create a PR if we decide to do something about this.

jrtom commented 6 years ago

I don't particularly consider JUNG to be an extension to common.graph, but a client of it, but that's a minor issue. I agree that their style and practices are generally worth emulating, though.

I don't really care about backwards incompatibility in this context, because 3.0 is one ginormous breaking change as it is, and we're already going to need to write a migration guide. :)

Guava has used XUtils as a class name for things pertaining to X in the past, but it's true that they don't do that any more.

I agree that we should not have two different static-utility-method-holding classes named Graphs, that's just confusing.

Option (1) is fine by me.

jbduncan commented 6 years ago

Great! Thanks for your response @jrtom. Apologies for only responding just now, this issue fell off my radar!

I've had another look at TreeUtils (to be renamed to Trees). and I've noticed two functions -TreeUtils#roots and TreeUtils#isForestShaped - which I believe would be a better fit under Graphs (to be renamed to MoreGraphs), as they seem to operate on and be applicable to Graphs in general, as opposed to CTrees specifically. I'd quite like to hear your thoughts on whether it would be a good idea to move those functions.

jrtom commented 4 years ago

Only ~2 years later (sorry about that)...yes, I think your suggestion of moving those specific methods to MoreGraphs is reasonable.

That said, a related issue we may want to tackle (either separately or in concert), brought up here: https://github.com/jrtom/jung/pull/244#pullrequestreview-319577778

tl;dr: we're inconsistent in our use of the term "Tree" in our APIs. :P