Guava: Please make graphs and graph builders public and non-final

[NateUniAccount]

Great library!

I'd like to extend a graph class so as to replace the huge type name it generates with a shorter one more germane to my usage. When a graph gets built, it's type name is something like MutableValueGraph<SourceElement, SourceRelationship> whereas I'd like to extend it with a class called SourceGraph...much better! The graph builder classes don't allow changes to build a custom graph unless you're happy re-inventing the wheel so it's a case of copy-paste the code just to use the shorter name or use the huge name everywhere.
If the classes were extendable my use case would be very easy.

If there's a good reason not to I don't know it and would love to hear it.

[cpovirk]

Extending classes to provide typedef-like functionality usually doesn't go well: As soon as you call a method like transitiveClosure, you have a Graph that isn't of the right type anymore.

You could consider creating a SourceGraph type that contains an instance of MutableValueGraph<SourceElement, SourceRelationship>, but of course then you either need to copy the methods you're interested in or else refer to sourceGraph.theActualGraph.someMethod().

(That said, MutableValueGraph is an interface, so I think you could create a subtype of that if you wanted. That doesn't help with types like ImmutableGraph, but we'd want to be especially careful in letting anyone subclass those, since people might break immutability guarantees.)

[jrtom]

Side note: generally speaking, the only time that one should be working with the Mutable types is when they are modifying the graph, or are returning a graph that their callers will expect to be modifiable. Otherwise you should (in this case) be using ValueGraph<...>.

(ValueGraph and its siblings are, of course, also interfaces that you can create subtypes of.)

[NateUniAccount]

To be fair, calling transitiveClosure doesn't respect the original graph class anyway even if it's a guava graph class. Yeah forwarding or copy-paste seem to be the only options but I was kinda hoping to avoid them. Think I'm gonna go with jgrapht because it's a bit more open even though it's not as clean.
Anyway thanks for the quick reply, much appreciated!

[jrtom]

Not sure what you mean by saying thattransitiveClosure "doesn't respect the original graph class". It necessarily creates a new graph instance, so as to avoid mutating its input. How would you expect it to work, and why would you expect it to return a particular type other than Graph<N>?

For what it's worth, we've considered polishing up and making public the Forwarding* classes that common.graph currently uses internally for things like Graph.transpose(). If that would make your life easier, please file an issue to that effect. Thanks.

[jbduncan]

I'm kind of curious to know as well what you mean by "transitiveClosure doesn't respect the original graph class". Did you mean that you expect the returned graph to be of the exact same type as the original graph? Or did you mean something else?

[NateUniAccount]

I don't have any expectations about the method, just mentioned it because it was put forward as a reason against allowing extending the Graph sub-classes, when the same can be said of the other sub-classes.
I'm not trying to change how you do things! This was literally just to save me typing half a dozen words every now and then lol
FWIW there are several ways it could return the same type but am not sure they'd add enough value, a few off the top of my head:
Overriding clone()
Calling graph.getClass().newInstance()
Adding a typed createBlankGraph() method
Storing a Supplier in each graph that creates a blank graph
etc.

[cpovirk]

I'm to blame for starting that part of the discussion :) I probably overstated things. I know I have seen problems in the past when people (including me) tried a "typedef" like this, but it's been a while, and I don't remember the details well. So while I still wouldn't recommend the pattern in general, it might well work OK in your case.

(I managed to dig up The pseudo-typedef antipattern about the general pattern.)

[jrtom]

@NateUniAccount Thanks for the additional context. A few quick responses for future reference:

• clone() seems like a bad fit because that implies "create a copy of this object" rather than "create a new instance of this object's type".
• graph.getClass().newInstance() would be a better fit conceptually, but I don't like using reflection unless it's absolutely necessary.
• I'm not sure what you'd propose adding a "typed createBlankGraph()" method to in the common.graph library. It's certainly something you could create for yourself, of course (and that's probably the best solution IMO).

For those who may be reading this thread and wondering what the best way of constructing a common.graph graph instance of the same type as an existing graph, it's this:

Graph<N> newGraph = GraphBuilder.from(existingGraph).build();

We might at some point add a builder() method to the graph interfaces to provide an alternate (and perhaps easier?) way of doing this:

Graph<N> newGraph = existingGraph.builder().build();

(I won't comment further on the "typedef" thing specifically, as I think that @cpovirk has that covered.)

[Maaartinus]

@jrtom

I'm not sure what you'd propose adding a "typed createBlankGraph()"

I'd bet, he meant an instance method returning a new empty instance (just like graph.getClass().newInstance(), but implemented any way you like). Anyway, given the GraphBuilder, it's not needed.

We might at some point add a builder() method to the graph interfaces to provide an alternate (and perhaps easier?) way of doing this:

IMHO it's much easier, but maybe toBuilder() would be a better name as builder() is normally the static method (and some classes might need both).

[jrtom]

The benefit of having an instance method that would return a new empty instance, versus GraphBuilder, would be that you could have it return your particular subtype of Graph (or whatever); the original concern was that since GraphBuilder is final that he couldn't create his own subclass of it that would return his own Graph subtype.

I agree that toBuilder() is a better name for that hypothetical method.

[Maaartinus]

Damn... I guess, I should get more sleep. ;)