-
Notifications
You must be signed in to change notification settings - Fork 1k
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
base: dev-2.x
Are you sure you want to change the base?
Move rounding of WgsCoordinate from constructor to factory method #6014
Conversation
Codecov ReportAttention: Patch coverage is
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. |
this.latitude = DoubleUtils.roundTo7Decimals(latitude); | ||
this.longitude = DoubleUtils.roundTo7Decimals(longitude); | ||
this.latitude = latitude; | ||
this.longitude = longitude; | ||
} | ||
|
||
public WgsCoordinate(Point point) { |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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 ?
There was a problem hiding this comment.
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.
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 theWgsCoordinate.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:
WgsCoordinate.of(Coordinate)
andWgsCoordinate.of(Point)
constructors will throw exceptions if the lat/lon is out of bounds.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.