Refactor Building#getLastKnownDistance methods

[muandrew]

resolves https://github.com/OpenBW/BWAPI4J/issues/49

no changes have been made to the final calculations, but should be trivial to change.

[adakitesystems]

Hi, @muandrew! Thank you for the PR!

The result looks better! Nice job!

However, there are a few "hidden" changes in this PR which I dislike; namely the usage of annotations, the confusing method getApproxDistance, and importing another assert framework.

1) Annotations I like the idea of distinguishing between the different position precision levels such as Position, WalkPosition, and TilePosition for the x and y values using @Tile etc., but we do not feel the annotations help that much. It is mostly for documentation and does not prevent the programmer from mixing an x from a TilePosition with an x from a Position. I would agree it's possible it might be slightly more difficult to switch them if the programmer has to consciously write @Tile int x instead of @Walk int x and then a warning/error is displayed. However, I feel it adds more clutter than good it does. I consider @Tile int x roughly the same as int tileX. Unless I misunderstood or you can argue the usage of these specific annotations further, we ask that you please remove them in this PR.

2) getApproxDistance In BWAPI C++, getApproxDistance is a member of the position classes. You have added getApproxDistance as a static function and also changed its parameters from plain x and y values to the deltas of x and y; forcing the user to calculate the deltas first. I highly disagree with this because of how similar the name is to the original BWAPI C++. I see you used it in the refactoring. Maybe it should be a private static function or renamed to getApproxDistanceUsingDeltas.

3) Google Truth While the argument of using Google Truth over the existing assert framework is entirely justifiable, it's a hidden change and also now adds yet another dependency. The usage of Google Truth should be a separate PR. Is Google Truth worth refactoring all of the current tests? Or leave the current tests as is and start using Google Truth?

[muandrew]

Hey thanks for the response. I didn't mean for any of the changes to be hidden. Each independent change was broken out into its own commit and shouldn't hide or obsfucate the intention of the changes. I do agree that this PR would be doing too much if the repository was configured to squash all PR's into one single commit upon acceptance. I however, really should have explained some of the changes.

1) Annotations I agree with them looking clunky. They can be enforced however, with linters but since I probably won't get that functionality done anytime soon I don't mind removing them. What would be nice is if this library used a language that has type aliasing (like Kotlin!). Can I however, rename the methods to getTileDistance getWalkingDistance, instead of a single overloaded getDistance? I think that will reduce the amount of errors users make. One more question, would you prefer the return types to be int or double?

2) getApproxDistance The reason why it's public is because right now there are two ways to calculate distance that I didn't want to change in this PR. One is square root and one is this approximation. Ideally I would just have one calculate distance method with eight parameters for each of the boundaries of the source and target. If you would prefer the calculations to be changed, I can consolidate them into this PR. I however don't agree with getting users confused with the cpp method since this was created to emulate the same behavior and users of the library will probably not know that a version exists in cpp.

3) Google Truth This dependency does not introduce any additional weight into the library. It does however simplify how unit tests are written. You no longer have to write the message into every method, but I can break that out into another PR.

Assert.assertEquals("expected: " + LAST_KNOWN_DISTANCE_SAMPLES_BY_TILEPOSITION[index] + ", actual: " + building.getLastKnownDistance(tpos), true, Integer.compare(LAST_KNOWN_DISTANCE_SAMPLES_BY_TILEPOSITION[index], building.getLastKnownDistance(tpos)) == 0);
assert_().that(building.getLastKnownDistance(tpos)).isEqualTo(LAST_KNOWN_DISTANCE_SAMPLES_BY_TILEPOSITION[index]);

I would recommend that the migration to Truth be gradual. You can also look into AssertJ. Here is a link comparing the two. https://google.github.io/truth/comparison

[adakitesystems]

Maybe "hidden" changes was possibly a bit vague and accusatory. I wasn't insinuating you purposely meant to hide anything. I acknowledge the mentioned changes were clearly marked and not "hidden". However, I meant they are unrelated to the PR. A set of methods were to be refactored but now the usage of annotations has been injected and a new assertion framework has been introduced. Those two are completely separate and somewhat major design changes/decisions. I concede the entire codebase is not 100% consistent, but with these new additions, it becomes even more inconsistent in the name of refactoring but a few methods.

What would be nice is if this library used a language that has type aliasing (like Kotlin!)

@Bytekeeper's bot was written in Kotlin if you'd like to talk to him about possible approaches/solutions.

1. Annotations. @Bytekeeper and I discussed these proposed annotations and voted 2-0 against. We both think the usage of annotations in general could be beneficial, but that should be left for another discussion another time.

2. getApproxDistance Because the userbase of BWAPI is majority C++ and the majority of the libraries/extensions/codebase is in C++, it is wise to keep the API and its method names/definitions/results as close as possible to the original C++ API. This helps with collaboration and compatibility. Also, a lot of Java users will ask for help in the unofficial BWAPI discord of which the users are predominately C++. Their advice is useless or confusing if the Java API is completely different or if a method has the same name but a different definition regardless if the Java version fixed any C++ design pitfalls. If BWAPI4J was the only BWAPI project around, I would agree a lot of things should be different. In this case, it is a hard "no" from me to change the definition of getApproxDistance to accept deltas instead of plain x and y values. I think getTileDistance or getWalkingDistance is a good idea to be added/extra functions of the position classes. However, that is a separate issue.

3. I agree Google Truth looks promising and more improved/modern. However, the tests were originally written with JUnit and the format "expected, actual". The C++ Google Test format is also "expected, actual". E.g. EXPECT_EQ(expectedValue, actualValue). The Truth format is assert.that(actual).equals(expected). I also haven't looked at its source. For now, it seems simpler to just use the "expected, actual" format. I acknowledge this new framework doesn't add any bloat to the distributed library. However, it adds yet another download to Gradle. I think we have too many already and want to get rid of some if possible.

In summary, thank you very much again for refactoring these methods and for the detail explanations. I request the annotations be removed, change getApproxDistance to match the C++ API, and remove Google Truth.

Because I have requested so much to be removed from this PR, if you like, I could submit a PR to your changes which should better clarify my request so we can reach an agreement.

One more question, would you prefer the return types to be int or double?

I noticed that some methods that return integers should actually return double and/or vice versa. This is probably a mistake. I believe the return types should match the C++ API.

[muandrew]

No need for double pull requests, I'll break this up first. I agree that these should be separate matters since they need different discussions.