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

Move rounding of WgsCoordinate from constructor to factory method #6014

Draft
wants to merge 2 commits into
base: dev-2.x
Choose a base branch
from

Conversation

habrahamsson-skanetrafiken
Copy link
Contributor

@habrahamsson-skanetrafiken habrahamsson-skanetrafiken commented Aug 21, 2024

Summary

This is part 1 of some changes to the coordinate classes.

This PR converts the WgsCoordinate into a record and changes the default constructor to not round the coordinates to 7 decimal places. If you want the rounding behaviour you instead call the WgsCoordinate.normalized(lat, lon) factory method.

The main reason for this change is that we want to change a lot of code to start using the WgsCoordinate class without having to change the behaviour of the existing code.

This code is mostly a refactor. But there are some changes in behaviour:

  • Now the WgsCoordinate.of(Coordinate) and WgsCoordinate.of(Point) constructors will throw exceptions if the lat/lon is out of bounds.
  • I didn't update all creations of the WgsCoordinate to use the normalizing factory method. A lot of test code is doing new WgsCoordinate(1, 2), i left those as they are.

Issue

See #6013

Unit tests

I updated some test code in the WgsCoordinateTest according to the new behaviour.

Documentation

I added javadoc to the new factory methods.

Bumping the serialization version id

Converting the WgsCoordinate into a record will probably change the serialization.

Copy link

codecov bot commented Aug 21, 2024

Codecov Report

Attention: Patch coverage is 94.00000% with 3 lines in your changes missing coverage. Please review.

Project coverage is 69.73%. Comparing base (ff838eb) to head (667a7ed).
Report is 41 commits behind head on dev-2.x.

Files with missing lines Patch % Lines
...ain/java/org/opentripplanner/model/plan/Place.java 70.00% 1 Missing and 2 partials ⚠️
Additional details and impacted files
@@              Coverage Diff              @@
##             dev-2.x    #6014      +/-   ##
=============================================
- Coverage      69.73%   69.73%   -0.01%     
- Complexity     17315    17316       +1     
=============================================
  Files           1960     1960              
  Lines          74263    74268       +5     
  Branches        7603     7603              
=============================================
+ Hits           51791    51792       +1     
- Misses         19831    19833       +2     
- Partials        2641     2643       +2     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

this.latitude = DoubleUtils.roundTo7Decimals(latitude);
this.longitude = DoubleUtils.roundTo7Decimals(longitude);
this.latitude = latitude;
this.longitude = longitude;
}

public WgsCoordinate(Point point) {
Copy link
Member

Choose a reason for hiding this comment

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

I would prefer to make all constructors private to enforce the use of the factory methods, this is a minor thing and we could do it later in another PR if that is more convenient.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Just curious, what is your reasoning for preferring factory methods in this case?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I converted the WgsCoordinate into a record and kept only the canonical constructor, what do you think about that @t2gran ?

Copy link
Member

@t2gran t2gran Sep 5, 2024

Choose a reason for hiding this comment

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

As discussed in the dev-meeting, I do not want this as a record.

  • We have a policy of not using records for domain classes - this is an edge case - so might be ok...
  • For memory/performance reasons I would like to hide the fields lat/long - we might convert these to int at some point.
  • The toString() should be kept(value-object), the hc/eq are simple - not much to gain. The proper lack of encapsulation and the copy-existing-code risk out-weight the reduction in code size.

@habrahamsson-skanetrafiken habrahamsson-skanetrafiken added the bump serialization id Add this label if you want the serialization id automatically bumped after merging the PR label Aug 29, 2024
@habrahamsson-skanetrafiken habrahamsson-skanetrafiken marked this pull request as draft September 5, 2024 13:13
@t2gran t2gran added this to the 2.7 (next release) milestone Sep 18, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bump serialization id Add this label if you want the serialization id automatically bumped after merging the PR Skip Changelog
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants