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

Kotlin implementation of https://github.com/heremaps/flexible-polylin… #79

Open
wants to merge 5 commits into
base: master
Choose a base branch
from

Conversation

moo24
Copy link

@moo24 moo24 commented Jun 4, 2024

Kotlin implementation of Flexible Polyline encoding based on Java implementation.
API wise, I implemented the Dart API as I prefer that one.

Note: There are currently still dependencies to java.util.concurrent.atomic which should be replaced with kotlin.native.concurrent or a project like https://github.com/Kotlin/kotlinx-atomicfu in case the code shall be used with a kotlin multiplatform project.

Tests are passing. Performance on JVM seems worse (about 4x) than pure Java implementation; so there is room for improvement. However, intention is to provide a kotlin implementation in case someone needs it (e.g. for Kotlin native projects).

@cio
Copy link
Contributor

cio commented Jun 5, 2024

Hi,

Is there a specific reason why you used the AtomicX classes? An old version of the Java implementation had them and let's say just that I am glad that we got that part completely re-written.

@moo24
Copy link
Author

moo24 commented Jun 7, 2024

Thank you for the hint. I created the first implemenation some time ago, but never got to contribute it to this project. I now refactored it to also remove the atomic.* references. A nice side effect - the code performance improved.

…e/tree/master/java

Note: There are currently still dependencies to java.util.concurrent.atomic which should be replaced with kotlin.native.concurrent  or a project like https://github.com/Kotlin/kotlinx-atomicfu in case the code shall be used with a kotlin multiplatform project.

Performance on JVM seems worse (about 4x) than pure Java implementation; so there is room for improvement.

Signed-off-by: Hille, Marlon <marlon.hille@here.com>
…me code clean-up

Signed-off-by: Hille, Marlon <marlon.hille@here.com>
@moo24
Copy link
Author

moo24 commented Jun 12, 2024

Fixed the missing commit sign-off.

Copy link
Contributor

@cio cio left a comment

Choose a reason for hiding this comment

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

Looks good. Should be polished a bit more.

kotlin/src/com/here/flexiblepolyline/FlexiblePolyline.kt Outdated Show resolved Hide resolved
kotlin/src/com/here/flexiblepolyline/FlexiblePolyline.kt Outdated Show resolved Hide resolved
kotlin/src/com/here/flexiblepolyline/FlexiblePolyline.kt Outdated Show resolved Hide resolved
kotlin/src/com/here/flexiblepolyline/FlexiblePolyline.kt Outdated Show resolved Hide resolved
kotlin/src/com/here/flexiblepolyline/FlexiblePolyline.kt Outdated Show resolved Hide resolved
kotlin/src/com/here/flexiblepolyline/FlexiblePolyline.kt Outdated Show resolved Hide resolved
kotlin/src/com/here/flexiblepolyline/FlexiblePolyline.kt Outdated Show resolved Hide resolved
@moo24 moo24 force-pushed the master branch 2 times, most recently from e950d05 to 38110f5 Compare June 25, 2024 13:43
Signed-off-by: Hille, Marlon <marlon.hille@here.com>
…able

Signed-off-by: Hille, Marlon <marlon.hille@here.com>
@moo24 moo24 requested a review from cio June 25, 2024 17:49
Copy link
Contributor

@cio cio left a comment

Choose a reason for hiding this comment

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

Thank you for the changes they look good. However, it could be polished a bit more.

Copy link
Author

@moo24 moo24 left a comment

Choose a reason for hiding this comment

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

Done

… Iterator interface

Cleanup FlexiblePolyline.Converter and FlexiblePolyline.ThirdDimension as there is no need to use the package name
Cleanup of comments
Using an Exception with more details in case loading a test case failed

Signed-off-by: Hille, Marlon <marlon.hille@here.com>
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.

None yet

2 participants