Normalize longitude without looping #460
Conversation
Change normalizeLongitude to remove loops. Returned value is a normalized longitude in range [-180;+180[
|
Note: #373 should probably NOT be closed by this, as there might be other implementations that still need to be fixed. |
| return longitude; | ||
|
|
||
| final long CIRCLE_DEG = 2 * LONGITUDE_MAX; // 360 degrees | ||
| return (longitude % CIRCLE_DEG + CIRCLE_DEG + LONGITUDE_MAX) % CIRCLE_DEG - LONGITUDE_MAX; |
There was a problem hiding this comment.
Can this just be (longitude + LONGITUDE_MAX) % CIRCLE_DEG - LONGITUDE_MAX? If not, please add a comment in the code about why, I got a bit stuck trying to figure it out.
There was a problem hiding this comment.
This is due to % in JAVA using truncated division, meaning that the remainder can be negative.
With a potentially very large negative input, just adding LONGITUDE_MAX once might not be enough to bring this into positive range, in which case the single % would lead to a still negative result and subtracting LONGITUDE_MAX would bring the value back into an invalid range < -180.
By doing %CIRCLE_DEG twice, we first bring the value into a range that is [-360..360[, so that the second % after adding 360 leads to a result that is definitely in range [0..360[.
I added a comment, please see if it explains this well enough. :)
comment on why we need to apply % twice, as requested
|
Please add a couple test cases to https://github.com/google/open-location-code/blob/main/test_data/encoding.csv -- it currently has test cases for ">=180", but nothing for ">360" or "<-360". Thank you! |
Splitting tests for longitude outside proper range (normalization needed) from those testing latitude clipping. Reusing tests from above by adding/subtracting multiples of 360.
Change normalizeLongitude to remove loops. Returned value is a normalized longitude in range [-180;+180[
I'm not sure how much commenting the final line really needs, so please advise.
If accepted, this fixes #373 for the Java implementation.