abrensch / brouter

configurable OSM offline router with elevation awareness, Java + Android
MIT License
474 stars 113 forks source link

Update new parameter collector for BRouter app #634

Closed afischerdev closed 7 months ago

afischerdev commented 9 months ago

Use the new parameter collector also in app context and remove old param logic and routines.

quaelnix commented 8 months ago

Lots of duplicate code gone for good! :+1:

... however, I'm a little afraid that this might break something, though I haven't found anything yet.

afischerdev commented 8 months ago

@quaelnix

however, I'm a little afraid that this might break something, though I haven't found anything yet.

Indeed it has the potential to run into problems.

My ideas on this situation:

In my opinion, the server is not critical. It could change the library very quickly if something doesn't work properly. And these kind of changes can be easily tested by comparing the results - there should be no change. But for the app an update on trouble will take some days. And this is not easy on testing.

It could be a good way to use a beta app via the Play Store to have more test with users. But we can't publish an update without any user relevant changes. So I would like to add changes on voicehint #613 as well.

quaelnix commented 8 months ago

add a new output logic (move from OsmTrack to extra classes) - this also carries a risk in it

If it improves the readability of the code, I'm in.

In my opinion, the server is not critical. It could change the library very quickly if something doesn't work properly.

I think we should try to break neither of the two. We can't know who is using the server in what way.

afischerdev commented 8 months ago

If it improves the readability of the code, I'm in.

It only clears the OsmTrack from formatting and move it to Gpx, Kml, Json, Cvs classes. But code is as it was before. Map matching will need 5x5 OsmTracks to compare each point with next.

I think we should try to break neither of the two.

Yes, of course, but how do you achieve this goal?

afischerdev commented 8 months ago

@quaelnix

I think we should try to break neither of the two.

Is there a new idea to protect the server lib or/and the app from breaking?

zod commented 8 months ago

Getting rid of duplicated code is always desirable because it's less code to maintain :) Thanks for your effort!

The usual approach to ensure refactoring doesn't break functionality is to write tests before refactoring which represent the desired behavior. Unfortunately most BRouter classes can't be tested in isolation because of close-coupling with other classes which makes writing unit tests really hard. Usually coverage information is also used to have a metric/information which parts of the code are covered by tests.

afischerdev commented 8 months ago

@zod My extra tests are no unit tests. I use the server and fire around 80 requests for old lib against new lib. And compare the results. For the parameter and the new output logic the tests are easy. The result must be the same. That covers the most features we have. Some day I like to do that with an additional Android app as well.

quaelnix commented 8 months ago

Is there a new idea to protect the server lib or/and the app from breaking?

No, and I also do not understand this part of the code well enough to be of any help in this matter. Sorry.