LatLong.compareTo() fix by Sublimis · Pull Request #1628 · mapsforge/mapsforge

Conversation

@Sublimis

LatLong.compareTo() was incorrectly implemented, so it could happen that e.g. latLon1.compareTo(latLon2) == latLon2.compareTo(latLon1) == 1 for coordinates latLon1=(1, 0) and latLon2=(0, 1).

Although the fix is ​​crucial, LatLong.compareTo() does not seem to be used at the moment (is this correct?), so it may not change any current behavior of the library.

@devemux86

@Sublimis Do you think we need some tests in LatLongTest class?

@Sublimis

@Sublimis Do you think we need some tests in LatLongTest class?

This could be useful, yes. Some tests will be crafted.
It's interesting to note that if LatLong were to extend from org.mapsforge.core.model.Point, and encapsulate the fields with getters/setters, we'd have all this automatically :)

@devemux86

LatLong comparison looks similar to the Point comparison,
except that Point compares the x / longitude coordinate first.

@Sublimis Should we compare latitude first in LatLong?

@Sublimis

@devemux86 This probably wouldn't matter while running, but if you think it improves consistency, that's good. It's interesting to note how the latitude always comes first, it's always latlon, not lonlat.

What do you think of this cleanup now?

@devemux86

It's interesting to note how the latitude always comes first, it's always latlon, not lonlat.

While it's usually x,y, coordinates in mapping are often latitude,longitude.

What do you think of this cleanup now?

@Sublimis Looks good, we can merge it if it's ready.

@Sublimis

@devemux86

Labels

2 participants

@Sublimis @devemux86