search

jrtom / jung

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

Create events only when necessary #85

Closed javagl closed 5 years ago

javagl commented 7 years ago

Admittedly, one purpose of this issue is to just mention that it's great to see that someone takes the lead on further development of JUNG. It's a really powerful library that easily allows creating a nice graph visualization quickly, and still offer configurability for more sophisticated tweaks. I hope that I can allocate some time to have a closer look at the refactorings that are leading towards JUNG 3.0, and maybe post some thoughts about the design choices.

Now, to the point that this issue is actually about: It's comparatively minor, but may become relevant for large and/or very dynamic graphs: In addEdge and addVertex of https://github.com/jrtom/jung/blob/common.graph/jung-api/src/main/java/edu/uci/ics/jung/graph/ObservableNetwork.java#L67 , the event is generated even if it is not used. Changing

if (state) { ... }

to 

if (state && !listenerList.isEmpty()) { ... }

would avoid the creation of garbage events.

Beyond that, I could point out (many) other minor issues, like

but I just had a short glance at the code, and this sort of nitpicking may not be appropriate....

jrtom commented 7 years ago

@javagl, there are many different things that could be done to make JUNG 3.0 better, even after the data model migration (to use Guava's common.graph types) is complete. I would be happy to get pull requests against the common.graph branch for issues such as the ones you're describing. I'd also be open to considering broader design questions.

As a relatively minor example, I plan to refactor a lot of the algorithms classes that have multiple constructors, each of which takes multiple parameters, to use a Builder pattern, to simplify the classes, and make the call sites more self-documenting. But I've got a very long list of things that need attention, and would love to get more people involved.

javagl commented 7 years ago

It's hard to promise any longer-term committment. The effort for contributions increases drastically...

But in any case, I'll try to fork and at least have a look at the current state.

I assume that the "list of things that need attention" manifests itself as the issues that you're opening? Or is there any other resource that summarizes the plans and design considerations?

jrtom commented 7 years ago

Belated response:

I don't insist that anyone that wants to contribute has to sign on for a seven-year tour. :)

For things like the issue you brought up above, a pull request seems like a good idea; it's a small, self-contained change that is pretty uncontroversial. I'd be happy to review such a change.

I have not opened issues for all of the things that I think need attention; far from it. I currently have a long doc that lists most of them, and I need to convert that into GitHub issues; thanks for prodding me on that point. :) I should probably also create a summary doc that talks about some overarching principles for the refactor.

And yes, I've been working on the JUNG 3.0 refactoring for at least several months now, and probably won't be done for a while yet, because I don't have a lot of time to devote to it.

javagl commented 7 years ago

The "minor issues" that I listed above show that this may be subtle:

However, with a certain awareness about source compatibility, one should be able to avoid breaking changes. So if even minor cleanups are welcome, then I'll more seriously consider issuing PRs. (I'd really like to familiarize myself more with the 3.0 state, and this could go hand in hand).

Beyond that, any roadmap, known issues or implicit "TODOs" could be helpful.

What also could be interesting: Some sort of guideline regarding the implementation style. Related to the example above: The pure, clean OOD principles dictate that every aspect of a class should be as private and as final as it can possibly be. But much of the flexibility of JUNG exactly came from ignoring this: One could override everything.

Of course, in many places, this flexibilty had been made explicit: The configurability could often not only be achived by overriding, but also by setting the appropriate Transformer (aka Function).

But these are points that may already become clearer when looking more closely at the last 3.0 refactorings.

jrtom commented 7 years ago

I didn't mean to imply that all of the hypothetical changes you suggested were ones you should start sending out PRs for, just the one that this issue was titled for. Other stuff should be proposed and decided on its own merits. :)

So, yes, do send out PRs for minor no-downside changes, thanks.

Regarding breaking changes: the transition to JUNG 3.0 is by definition a widespread breaking change (just as JUNG 1.x->JUNG 2.0 was). One of the things that we'll need to do (I've made a start) is create a migration guide to help people navigate that transition.

In any event, I'm taking advantage of this to do a lot of API cleanup.

I do want to retain at least most of the flexibility, but I'm open to options as to how best to accomplish that, and I think that it's probably best to not provide multiple ways of achieving the same ends in that regard.

javagl commented 5 years ago

Wow. More than two years. Occasionally, I considered taking a closer look at this repo in general (and IIRC, I linked to it as a "heads up" when the maintainer of https://github.com/tomnelson/jungrapht-visualization posted something on some Q/A site), but ... time.

As far as I can see, the solution by @wumpz just moves the !...isEmpty() check into the fire... method, adding an indirection (and thus, a tad of complexity that I don't immediately see an advantage of) via the Supplier. But it achieves the goal of not creating unnecessary events, so this issue can indeed be closed. (The "minor issues" that I mentioned in the initial post are generic and unrelated to this one, so to speak)

wumpz commented 5 years ago

Maybe the Supplier thing is a bit overdesigned. The idea is to take all the responsibility to perform additional checks away from the caller of the fire method while giving the caller the freedom to create any event it wishes.

jrtom commented 5 years ago

@javagl yeah, there's been a lot going on in my other work that has made it hard to find time to work on JUNG recently, so I especially appreciate @wumpz coming in and providing some energy. :)

I'm not sure that the Supplier mechanism is the best, either, but I believe that the general strategy (as @wumpz said) of handling the additional checks internally to the library is the correct one. Also, to be fair, @wumpz is also solving an additional problem--that of creating an event object when it's not needed--although one could argue that that's not a big deal.

I think that a viable alternative mechanism--considering that fireGraphEvent is protected and has only four call sites--would be to have the callers check whether there are any listeners before (creating the event object and) calling fireGraphEvent, thus:

public boolean addNode(N node) {
  boolean added = delegate.addNode(node);
  if (added && !listenerList.isEmpty()) {
    fireGraphEvent(new NetworkEvent.Node<>(delegate, NetworkEvent.Type.NODE_ADDED, node));
  }
  return added;
}

The tradeoff there is avoiding a method call versus a slight increase in implementation complexity and fragility. Not a big deal either way, I think.

If someone wants to send this as a PR I'd be willing to go with the above solution, but I'm not strongly motivated to change it.