Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

long to int coordinates, refactoring #395

Merged
merged 13 commits into from
Jul 16, 2021
Merged

Conversation

rtroilo
Copy link
Member

@rtroilo rtroilo commented Jul 13, 2021

This PR change the internal representation of the "osm-coordinates" from a long to int data-type.
It mainly affects the OSHDBBoundingBox class and a new OSMCoordinates helper class .

The OSHDBBoundingBox had 3 construtors with different data-types as arguments:

  • double type arguments, lon/lat wgs48 representation e.g. 8.6724, 49.3988
  • int type arguments, also lon/lat wgs48 but omitting the decimal parts, e.g. 8, 49
  • long type arguments, the interal "osm-coordinates", e.g. 86724000, 493988000

For better distinction, new constructor functions are introduced,

  • bboxLonLatCoordinates, for lon/lat coordinates
  • bboxOSMCoordinates, for "osm-coordinates"

And some helper methods

  • OSMCoordinates.toDouble -> from "osm-coordinate" to lon/lat coordinate
  • OSMCoordinates.toOSM -> from lon/lat to "osm-coordinate"

The refactoring where mostly done with search&replace like

var bbox = new OSHDBBoundingBox(-180, -90, 180, 90); // before
var bbox = OSHDBBoundingBox.bboxLonLatCoordinates(-180.0, -90.0, 180.0, 90.0); // after

OSHDBGeometryBuilder also got a new helper method

  • getCoordinate which converts a "osm-coordinates" into jts.Coordinates

other refactorings

OSHEntity

  • getNodes, getWays don't throw an exception any more!
  • code refactoring to avoid duplicated codelines

OSHDBHandler

The OSHDBHandler class in the etl module got some refactoring, mainly to get rid of duplicated codes sonar-lint warnings. For than a new OSHDBHandlerTest class were written.

Corresponding issue

Closes #320

Checklist

@rtroilo rtroilo changed the title Long to int coordinates 🚧 long to int coordinates, refactoring Jul 13, 2021
@rtroilo rtroilo requested a review from tyrasd July 13, 2021 09:26
@rtroilo
Copy link
Member Author

rtroilo commented Jul 13, 2021

There are some minor code cleanups in e.g. MapReducer

 (new TagTranslator(c)).close(); // before
 new TagTranslator(c).close(); //after

I could revert this, but I think there a reasonable, let me know.

Copy link
Member

@tyrasd tyrasd left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

At first glance this one looks good, I'll take a deeper look this afternoon. Can you in the meantime add a line or two to the changelog please, so this is ready to be merged?

@rtroilo
Copy link
Member Author

rtroilo commented Jul 15, 2021

At first glance this one looks good, I'll take a deeper look this afternoon. Can you in the meantime add a line or two to the changelog please, so this is ready to be merged?

Sure, thank you.

Please while you do the review, have a deeper look/critical eye on the changes within the utils/geometry packages. As there are not just search&replace changes. The test run through, though.

Also I'm not happy with the naming of the helper function OSMCoordinates.toDouble. What do you think about toLonLat,toWGS48, or similar.

@rtroilo rtroilo changed the title 🚧 long to int coordinates, refactoring long to int coordinates, refactoring Jul 15, 2021
Copy link
Member

@tyrasd tyrasd left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Already some general remarks (mostly inline, and one additional one below).

  • couldn't some of the parameters of the ZGrid class also be downgraded to int from long (e.g. as in … int denormalizeLon(int lon) { …)?

Please while you do the review, have a deeper look/critical eye on the changes within the utils/geometry packages. As there are not just search&replace changes.

I'm still taking a deeper look at these parts right now.

rtroilo and others added 2 commits July 16, 2021 11:02
…transform/oshdb/TransformOSHEntity.java

Co-authored-by: Martin Raifer <martin.raifer@heigit.org>
…Impl.java

Co-authored-by: Martin Raifer <martin.raifer@heigit.org>
@tyrasd
Copy link
Member

tyrasd commented Jul 16, 2021

Also I'm not happy with the naming of the helper function OSMCoordinates.toDouble. What do you think about toLonLat,toWGS48, or similar.

I agree that toDouble is not the best possible name. I'm also not sure what the best one actually is, but toWgs84 does sound ok for me.

@rtroilo
Copy link
Member Author

rtroilo commented Jul 16, 2021

I agree that toDouble is not the best possible name. I'm also not sure what the best one actually is, but toWgs84 does sound ok for me.

I renamed toDouble to toWgs84. It is definitive better then before.

@rtroilo rtroilo merged commit 75190d7 into master Jul 16, 2021
@rtroilo rtroilo deleted the long-to-int-coordinates branch July 16, 2021 14:38
@tyrasd tyrasd mentioned this pull request Aug 12, 2021
2 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants