Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Consider renaming TreeUtils and/or Graphs #166

Open
jbduncan opened this issue Dec 14, 2017 · 3 comments
Open

Consider renaming TreeUtils and/or Graphs #166

jbduncan opened this issue Dec 14, 2017 · 3 comments

Comments

@jbduncan
Copy link
Contributor

As I was working on tests for #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
Copy link
Owner

jrtom commented Dec 15, 2017

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
Copy link
Contributor Author

jbduncan commented Jan 9, 2018

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 jrtom added this to Internal cleanup in Prepare for JUNG 3.0 release Jan 6, 2019
@jrtom
Copy link
Owner

jrtom commented Jan 6, 2020

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: #244 (review)

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

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
Development

No branches or pull requests

2 participants