Java Implementation #21

Closed
wants to merge 2 commits into
from

Conversation

Projects
None yet
4 participants
@ianturton

This contains a java implementation of the OLC - it is based heavily on the Javascript implementation and passes all the tests in the csv files provided there.

Just run mvn install to test and release.

I've signed the CLA.

@drinckes

This comment has been minimized.

Show comment
Hide comment
@drinckes

drinckes Aug 19, 2015

Collaborator

Hi Ian - there was another Java implementation pull request that has been merged in. Could you see if there's any merging that makes sense?

Collaborator

drinckes commented Aug 19, 2015

Hi Ian - there was another Java implementation pull request that has been merged in. Could you see if there's any merging that makes sense?

@mprins

This comment has been minimized.

Show comment
Hide comment
@mprins

mprins Aug 19, 2015

Ian's pom file would definitely be nice to have/keep

mprins commented Aug 19, 2015

Ian's pom file would definitely be nice to have/keep

@ianturton

This comment has been minimized.

Show comment
Hide comment
@ianturton

ianturton Aug 19, 2015

I'm not sure I like the fact the all the methods return an Object that I have to call .getCode() on - seems redundant to me.
I don't like the CodeArea constructor (x,y,w,h) rather than (x1,y1,x2,y2) as it isn't intuitive or geographically sound.
Finally I saw the comment about rounding error from using doubles but I haven't seen any evidence for this so it's possible the overhead of using BigDecimals is unnecessary.

Functionally the two versions are compatible - so I could move the existing code around to make it work in Maven and probably keep my test class which uses the csv files rather than hard coding the test values.

I'm not sure I like the fact the all the methods return an Object that I have to call .getCode() on - seems redundant to me.
I don't like the CodeArea constructor (x,y,w,h) rather than (x1,y1,x2,y2) as it isn't intuitive or geographically sound.
Finally I saw the comment about rounding error from using doubles but I haven't seen any evidence for this so it's possible the overhead of using BigDecimals is unnecessary.

Functionally the two versions are compatible - so I could move the existing code around to make it work in Maven and probably keep my test class which uses the csv files rather than hard coding the test values.

@drinckes

This comment has been minimized.

Show comment
Hide comment
@drinckes

drinckes Aug 19, 2015

Collaborator

Now I've had a chance to look more closely, there are quite a lot of things I like about yours. The pom file will be useful because I want to set up build integration with travis-ci.org, so any pull request or push will trigger the tests. I also like your structure more (objects with static methods operating on them, rather than one big object).
I would ask that you use the existing test data files (in /test_data) if possible so tests can be added in one place if necessary.
But I'm ok if you do a fairly aggressive merge :-)

Collaborator

drinckes commented Aug 19, 2015

Now I've had a chance to look more closely, there are quite a lot of things I like about yours. The pom file will be useful because I want to set up build integration with travis-ci.org, so any pull request or push will trigger the tests. I also like your structure more (objects with static methods operating on them, rather than one big object).
I would ask that you use the existing test data files (in /test_data) if possible so tests can be added in one place if necessary.
But I'm ok if you do a fairly aggressive merge :-)

@drinckes

This comment has been minimized.

Show comment
Hide comment
@drinckes

drinckes Aug 19, 2015

Collaborator

I'll have some more comments tomorrow.

Collaborator

drinckes commented Aug 19, 2015

I'll have some more comments tomorrow.

@jirisemecky

This comment has been minimized.

Show comment
Hide comment
@jirisemecky

jirisemecky Aug 19, 2015

Contributor

Hi Ian,

I like the maven integration. To add some background - for the implementation, I used a different approach than you did - I wasn't looking at the JavaScript version and only followed the textual spec. I think this "double implementation" is good for catching possible bugs and not copying them over. I was actually able to catch inconsistencies between the test data and the spec.

I agree with your concern about the the intuitiveness of arguments of the CodeArea. These are just internal fields and "user" of the class doesn't see them and alway access them through the getters. But we can definitely change them to be the rectangle sides.

I like your idea of making the encode() methods return String directly in case this is what you want. Now they are there only to stick to the spec and just call the constructor.
However, I think the encapsulation in the OpenLocationCode is an advantage. If you only have Strings, you can never trust it and you always have to do the validity checks (which is an expensive operation). In my implementation the OpenLocationCode is actually just a glorified String, but guaranties to be a valid code, so you are sure all the operations pass on it.

Be careful about the statements about doubles, I run into this bug (the evidence) during implementation therefor I went for the BigDecimal. Floating points in java are tricky in this extend and they might not always add up correctly and in this case it matters if it's on the border of an rectangle. You can sometimes "hide" the problems on by shuffling the operant order but it is a time bomb. Your usage of the TOLERANCE constant suggest that you're just hiding the double problems. OLC doesn't limit the precision per spec, so this will hit you on some level.

For the tests, I also use the CSVs but I didn't want to copy/paste them for maintenance reasons, so I just refer to the files in test_data directory. Optimally, all the implementations should refer to the same golden test set.

Contributor

jirisemecky commented Aug 19, 2015

Hi Ian,

I like the maven integration. To add some background - for the implementation, I used a different approach than you did - I wasn't looking at the JavaScript version and only followed the textual spec. I think this "double implementation" is good for catching possible bugs and not copying them over. I was actually able to catch inconsistencies between the test data and the spec.

I agree with your concern about the the intuitiveness of arguments of the CodeArea. These are just internal fields and "user" of the class doesn't see them and alway access them through the getters. But we can definitely change them to be the rectangle sides.

I like your idea of making the encode() methods return String directly in case this is what you want. Now they are there only to stick to the spec and just call the constructor.
However, I think the encapsulation in the OpenLocationCode is an advantage. If you only have Strings, you can never trust it and you always have to do the validity checks (which is an expensive operation). In my implementation the OpenLocationCode is actually just a glorified String, but guaranties to be a valid code, so you are sure all the operations pass on it.

Be careful about the statements about doubles, I run into this bug (the evidence) during implementation therefor I went for the BigDecimal. Floating points in java are tricky in this extend and they might not always add up correctly and in this case it matters if it's on the border of an rectangle. You can sometimes "hide" the problems on by shuffling the operant order but it is a time bomb. Your usage of the TOLERANCE constant suggest that you're just hiding the double problems. OLC doesn't limit the precision per spec, so this will hit you on some level.

For the tests, I also use the CSVs but I didn't want to copy/paste them for maintenance reasons, so I just refer to the files in test_data directory. Optimally, all the implementations should refer to the same golden test set.

@drinckes

This comment has been minimized.

Show comment
Hide comment
@drinckes

drinckes Aug 21, 2015

Collaborator

My preference would be for Ian's version (sorry Jiri) due to it more closely matching the JS implementation. Jiri did mention a good point to me about the CodeArea object, that it is four arbitrary values and doesn't necessarily match an OLC box, but since the JS has the same issue I'm ok to live with it, and consider that issue as part of a new API at some later date.

Ian, could you please update when you're good to go (use the test_data directory files, remove the existing Java classes, use BigDecimal)?

Collaborator

drinckes commented Aug 21, 2015

My preference would be for Ian's version (sorry Jiri) due to it more closely matching the JS implementation. Jiri did mention a good point to me about the CodeArea object, that it is four arbitrary values and doesn't necessarily match an OLC box, but since the JS has the same issue I'm ok to live with it, and consider that issue as part of a new API at some later date.

Ian, could you please update when you're good to go (use the test_data directory files, remove the existing Java classes, use BigDecimal)?

@drinckes

This comment has been minimized.

Show comment
Hide comment
@drinckes

drinckes Sep 11, 2015

Collaborator

Actually - Jiri and Ian - can you collaborate on this? I'm coming around to a more OO view of implementations, where the class initialiser can be called with the arguments of encode, decode, and recover_nearest so those methods don't actually need to be exposed.

Maybe work on a fork and then send it back as a pull request when you agree?

Collaborator

drinckes commented Sep 11, 2015

Actually - Jiri and Ian - can you collaborate on this? I'm coming around to a more OO view of implementations, where the class initialiser can be called with the arguments of encode, decode, and recover_nearest so those methods don't actually need to be exposed.

Maybe work on a fork and then send it back as a pull request when you agree?

@drinckes

This comment has been minimized.

Show comment
Hide comment
@drinckes

drinckes Jan 20, 2017

Collaborator

I'm going to close this request since the existing Java implementation works and nobody's responded on this for months.

Collaborator

drinckes commented Jan 20, 2017

I'm going to close this request since the existing Java implementation works and nobody's responded on this for months.

@drinckes drinckes closed this Jan 20, 2017

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment