Feedback: Encapsulation issue, delegation and other thoughs after migration to 1.0

Hi guys.
I am migrating from 0.11 to 1.0. Generally nice progress, really appreciate your work, good idea with enums and encoded values.
But.
I have one request - please, think up encapsulation policy.
For example, if someone wants to provide his own algorithm factory which uses landmarks approximation… it is impossible to do it clearly:

  • you can make your class in package com.graphhopper.routing.lm because LandmarkStorage is package private
  • you can also make a workaround like use LMRoutingAlgorithmFactory, then make dummy algorithm like AstarBi and then extract resulting approximator from this algorithm.

Also.
I have noticed that in some places, where delegation was prefered, it dissapeared. For example in all places, except node based ch, weighting is wrapped in graph weighting. It is ok that it is wrapping, but… what is it actually doing? (also please notice https://github.com/graphhopper/graphhopper/blob/a5b48ceadc6b3973659075233687dd8991748d87/core/src/main/java/com/graphhopper/routing/ch/CHRoutingAlgorithmFactory.java#L58 getWeighting call and this bellow).
Not sure about TurnCostWeighting inlining in AbstractBidirCHAlgo but taking into account graph bellow and that node based routing cannot provide turn cost…https://github.com/graphhopper/graphhopper/blob/a5b48ceadc6b3973659075233687dd8991748d87/core/src/main/java/com/graphhopper/routing/AbstractBidirCHAlgo.java#L211

.
And the last - no SRP in fastest weighting, different approach in shortest. I will reference to my issue https://github.com/graphhopper/graphhopper/issues/1951.

You guys don’t have to take my words into account, but please consider that some users are not only using you service - also low level API, like me, which I am migrating for recent 2 weeks already, and long way ahead. Some changes are good, but others… would like to avoid.

Thanks for any response, maybe counterarguments or critism, and regards :slight_smile:

Hi, thanks for your feedback. There are indeed quite a few refactorings going on at the moment and more are already planned. First of all the current stable version is 0.13. There is no version 1.0 yet, so I am not exactly sure which migration you are planning to do. Migrating to any pre-releases is not a good idea in my opinion, as there are many changes some of which might be only temporary as well. Which parts of GH can be considered stable and which are subject to (drastic) change is always up to discussion and there is no clear policy for this yet. I think the best thing you can do is to describe in detail what kind of modifications, extensions etc. you are doing or planning to do with GH, such that such kind of use-cases might be considered in future developments. Maybe some of your ideas and changes might even be useful for the upstream project and it would be ideal to add them to the upstream project.

Regarding the things you mentioned here, honestly I have a hard time to follow what you are trying to say. Can you be more specific? What do you mean with ‘SRP’?

1 Like

Hi, thanks for you reply!
unfortunatelly, I need to migrate now - too much work now, and migrating to 0.13, to later migrate to 1.0… 2x times more work, especially I have a big layer upon GH.

By SRP I mean “single responsibility principle” - class should do just one thing, and nothing more; FastestWeighting by now not only calculatest fastest weight, but also takes into account penalties for private routes and destinations, and this behavior is not considered in ShortestWeighting for example, but is in ShortestFastestWeighting… because it inherits after FastestWeighting… some kind of a bad smell IMHO, even if it is configurable.

Regarding your request about beeing more specific - could you point out your issues about my statement? Don’t get me wrong - just want to avoid spamming.

Also, sometimes commit messages are very hard to follow - for example I have a long way to find where CHWeighting was removed. And in the same commit, to understand changes in AbstractBidirCHAlgo, which was not so quite new, but ‘introduced’ in https://github.com/graphhopper/graphhopper/pull/1850. You know, when I need to migrate some part of my code, and see that some class is no longer available, I need to know what to do in that case. And this way is not so handy :slight_smile:

FastestWeighting does one thing: It calculates the edge- (ok and turn-) weights to obtain the fastest route. And like any weighting it needs to deal with private road restrictions etc. as well (just like it is dealing with speeds etc.) But you are right ShortestWeighting does not do that and you already opened an issue for this.

I will try…

What needs to be changed to make it possible then? You have not really explained what you want to do so its hard to guess what needs to be changed. Note that the routing algorithm factory especially might be replaced or changed, because it has several drawbacks.

What is a ‘graph weighting’? What makes you think it wraps all weightings except in node based ch?

Sorry, but this does not seem to be a proper sentence and honestly I have no idea what you mean.

Yes we are trying to make GH also usable as a library on the low level API, but at the same time we are trying to implement new features etc. and there is always a trade-off between going forward and breaking things and trying to stay backward compatible which slows down progress and makes things often too complicated.

Some changes are good others you would like to avoid. But which ones do you like and which ones would you like to avoid, and more importantly why would you like to avoid them and what are better alternatives?

Yes especially in 1.0 there will be many such changes. So many that it became too hard for us to track all these low level changes. Maybe it would be useful to write some kind of migration guide that lists the most important of these changes. Did you somehow record the things you had to change? That would be a good start. The changes of the CH graphs and algorithms are mostly temporary and might only seem useful when they will be completed.

1 Like

Yes especially in 1.0 there will be many such changes. So many that it became too hard for us to track all these low level changes.

:+1: I just have to add that we did not gave any guarantees of a Java API stability (it is a 0.x version after all) and as long as there are no users reporting problems back this likely won’t change as we just can’t know if it is worth the effort at all and where to spend the actual time of documenting this.

I need to migrate now - too much work now, and migrating to 0.13, to later migrate to 1.0

In the current state you should NOT migrate to 1.0-SNAPSHOT, except you are willing to invest some more time to migrate to the final release. It is not released yet and things might still change. Also in general it is better to migrate as often as possible and from one release to another instead of a big jump.

And as @easbar already said: please contribute with your exact problems and how you fixed them and specify exactly what you need. Otherwise we won’t be able to help now and improve our migration strategy for coming releases.

1 Like
Powered by Discourse