Added checks for low point ammounts by boldtrn · Pull Request #1108 · mapbox/mapbox-navigation-android

boldtrn · 2018-07-12T14:33:01Z

While experimenting with the SDK I found two potential issues in the the code.

In NavigationMapRoute#addUpcomingManeuverArrow there is a check if the upcomingStepPoints are at least of size 2, but there is no check for the currentStepPoints. In obtainArrowPointsFrom the function TurfMisc.lineSliceAlong requires the points to be at least a size of 2. Both the upcoming as well als current steps are put into this method.

In OffRouteDetector#movingAwayFromManeuver I found a similar issue, there the stepPoints are check for the empty condition, but are put into TurfMisc.nearestPointOnLine which also requires at least a size of 2.

In both cases TurfMisc throws a TurfException which is not catched and can crash the SDK.

Unfortunately, I am not sure yet how to write a test for both cases :). If I am not mistaken, there are no tests for the NavigationMapRoute class yet? I had issues reproducing the case via OffRouteDetectorTest. So any hints regarding the tests would be highly appreciated.

danesfeder

@boldtrn great catch here and thank you for this contribution! We should definitely be checking for these cases before we use Turf.

I dropped some minor comments, mainly around readability / checkstyle. Once addressed we can cherry-pick your commit to a new branch so our CI will kick off. Thanks again, and let us know if you have any questions. 🚀

danesfeder · 2018-07-12T16:16:42Z


  private void addUpcomingManeuverArrow(RouteProgress routeProgress) {
-    if (routeProgress.upcomingStepPoints() == null || routeProgress.upcomingStepPoints().size() < TWO_MANEUVERS) {
+    if (routeProgress.upcomingStepPoints() == null || routeProgress.upcomingStepPoints().size() < TWO_MANEUVERS || routeProgress.currentStepPoints().size() < TWO_MANEUVERS) {


Our checkstyle runs a 120 character limit on each line, can we break this into two lines? Or better, can we extract each || into individual boolean conditions? For example

boolean invalidUpcomingStepPoints = routeProgress.upcomingStepPoints() == null || routeProgress.upcomingStepPoints().size() < TWO_MANEUVERS;

You are totally right, sorry I didn't think about a character limit.

danesfeder · 2018-07-12T16:18:13Z


  private void addUpcomingManeuverArrow(RouteProgress routeProgress) {
-    if (routeProgress.upcomingStepPoints() == null || routeProgress.upcomingStepPoints().size() < TWO_MANEUVERS) {
+    if (routeProgress.upcomingStepPoints() == null || routeProgress.upcomingStepPoints().size() < TWO_MANEUVERS || routeProgress.currentStepPoints().size() < TWO_MANEUVERS) {


For readability, can we create a new int constant here called TWO_POINTS that is used for:

routeProgress.currentStepPoints().size() < TWO_POINTS

Sure, not a problem, this seems to make the TWO_MANUVERS variable unused though, so just renamed it, I hope that's fine.

danesfeder · 2018-07-12T16:19:16Z

                                                Point currentPoint) {

-    if (routeProgress.currentLegProgress().upComingStep() == null || stepPoints.isEmpty()) {
+    if (routeProgress.currentLegProgress().upComingStep() == null || stepPoints.size() < 2) {


Same comments as above in NavigationMapRoute regarding this if check

Sorry I am not 100% sure what to do here :). Should I extract the 2 into a variable or convert the conditions into variable?

The line length looks alright, so I am not sure, if this is about shortening the line.

@boldtrn yep, exactly what you did with https://github.com/mapbox/mapbox-navigation-android/pull/1108/files#diff-cce5da198b8d3c30274778f11300d70bR448

Also replacing the 2 with TWO_POINTS as well.

Thanks for the clarification, I changed the code accordingly.

boldtrn · 2018-07-15T07:58:54Z

Thank you very much for the kind feedback @danesfeder. I updated the PR.

danesfeder

@boldtrn thanks for addressing the feedback here - I clarified my second comment. Once that's addressed this will be good to go! 💯

danesfeder · 2018-07-16T15:07:57Z

                                                Point currentPoint) {

-    if (routeProgress.currentLegProgress().upComingStep() == null || stepPoints.isEmpty()) {
+    if (routeProgress.currentLegProgress().upComingStep() == null || stepPoints.size() < 2) {


@boldtrn yep, exactly what you did with https://github.com/mapbox/mapbox-navigation-android/pull/1108/files#diff-cce5da198b8d3c30274778f11300d70bR448

Also replacing the 2 with TWO_POINTS as well.

Guardiola31337

Hey @boldtrn 👋 thank you for the contribution!

Unfortunately, I am not sure yet how to write a test for both cases :). If I am not mistaken, there are no tests for the NavigationMapRoute class yet? I had issues reproducing the case via OffRouteDetectorTest. So any hints regarding the tests would be highly appreciated.

Feel free to take a stab at it and push your changes so we can 👀 How does that sound?

Also I left some minor comments 😉

Thanks again 🙏

Guardiola31337 · 2018-07-16T15:38:58Z


  private void addUpcomingManeuverArrow(RouteProgress routeProgress) {
-    if (routeProgress.upcomingStepPoints() == null || routeProgress.upcomingStepPoints().size() < TWO_MANEUVERS) {
+    final boolean invalidUpcomingStepPoints = routeProgress.upcomingStepPoints() == null


Why does invalidUpcomingStepPoints need to be final?

I prefer to have variables immutable, if they don't have to be mutable. I can remove the final if you don't like it? I don't have a strong opinion on this.

Yeah, fair point. In the case of local variables, I personally prefer to avoid this. It causes visual clutter, and is generally unnecessary - a function should be short enough (preferably only doing one clear thing) that lets you quickly see that you are modifying something that you shouldn't. But yeah I agree this is a matter of taste 😅

For consistency with the rest of the code in which we're not including the final modifier for local variables, I'd say that removing it is the best solution to take here. What do you think?

Not a problem, I'll remove the final statement. Code should be consistent 👍.

Guardiola31337 · 2018-07-16T15:39:31Z

-    if (routeProgress.upcomingStepPoints() == null || routeProgress.upcomingStepPoints().size() < TWO_MANEUVERS) {
+    final boolean invalidUpcomingStepPoints = routeProgress.upcomingStepPoints() == null
+            || routeProgress.upcomingStepPoints().size() < TWO_POINTS;
+    final boolean invalidCurrentStepPoints = routeProgress.currentStepPoints().size() < TWO_POINTS;


Why does invalidCurrentStepPoints need to be final?

boldtrn · 2018-07-16T16:03:18Z

Thanks for the feedback @Guardiola31337.

Feel free to take a stab at it and push your changes so we can eyes How does that sound?

To be honest I already reverted my changes, I will take another chance tomorrow though :).

Guardiola31337 · 2018-07-17T08:45:59Z

To be honest I already reverted my changes, I will take another chance tomorrow though :).

That's awesome! Looking forward to 👀 them!

Thanks @boldtrn

boldtrn · 2018-07-17T13:19:36Z

  private Point lastReroutePoint;
  private OffRouteCallback callback;
  private RingBuffer<Integer> distancesAwayFromManeuver = new RingBuffer<>(3);
+  private static final int TWO_POINTS = 2;


This is duplicated now in OffRouteDetector and NavigationMapRoute, but since it's two different packages and the duplication is rather small, I prefered to duplicate it.

danesfeder · 2018-07-17T13:24:41Z

@boldtrn this looks good to me, thanks again for the work here!

@Guardiola31337 what do you think? 👍

boldtrn · 2018-07-17T13:43:13Z

Ok, I was able to produce a test. I guess the test could be done a lot nicer, but it's enough to show the issue. Please feel free to improve the test or maybe you could provide a hint or two how this could be improved.

So if you run the tests with fc8146a, you will get a TurfException, if you run the tests with e8d50da there is no exception.

Guardiola31337

@boldtrn

Ok, I was able to produce a test. I guess the test could be done a lot nicer, but it's enough to show the issue.

Thanks for adding a test checking the fixed issue ❤️

Please feel free to improve the test or maybe you could provide a hint or two how this could be improved.

Yeah I agree that this (as is designed right now) is hard to test (you may have noticed the huge amount of setup that is needed). This is telling us though that we should refactor OffRouteDetector to make it more testable. In any case, this is 💯 for now. We can revisit later.

Could you reformat the code (⌥⌘L if your are a Mac user 😅) and run our checkstyle task $> ./gradlew checkstyle to ensure that everything is aligned to our code guidelines?

Also left some minor comments. Other than that this is ready to go 🚀

Thanks again!

Guardiola31337 · 2018-07-17T18:37:53Z

  }

+  @Test
+  public void isUserOffRoute_StepPointSize() throws Exception {


Normally is good to structure the tests following the triple A 👀 https://speakerdeck.com/guardiola31337/elegant-unit-testing-mobilization-2016?slide=40 http://wiki.c2.com/?ArrangeActAssert
If the Arrange part gets too long we should create factory or builder methods.

Good point, I thought I had to delete the points, after the first off route check, it turns out, it's ok to do it before as well. Changed the order.

NIT - You can also remove some "unnecessary" new lines so that the 3 parts of the test (Arrange, Act and Assert) are visually 👌 i.e.

@Test public void test() { RouteProgress routeProgress = buildDefaultTestRouteProgress(); // ... boolean isUserOffRoute = offRouteDetector.isUserOffRoute(secondUpdate, routeProgress, options); assertFalse(isUserOffRoute); }

Sure that's not a problem.

Guardiola31337 · 2018-07-17T18:40:49Z

+    Point offRoutePoint = buildPointAwayFromPoint(stepManeuverPoint, 50, 90);
+    Location secondUpdate = buildDefaultLocationUpdate(offRoutePoint.longitude(), offRoutePoint.latitude());
+
+    // Manually remove all but one point to trigger the step point check


What about extracting this block of code into a private method and give it a name based on the comment? This way the test will be easier to read and understand and comment will become superfluous so it won't be necessary.

Sure, not a problem. Did that.

Guardiola31337 · 2018-07-18T07:46:10Z

Hey @boldtrn 👋 clean up / smaller commits like Show exception / Fix exception can be rebased for a cleaner history. Could you fixup and rebase? Thanks!

boldtrn · 2018-07-18T13:11:03Z

Could you fixup and rebase?

Sure, not a problem, should I just squash it into 1 commit?

Could you reformat the code (⌥⌘L if your are a Mac user sweat_smile) and run our checkstyle task $> ./gradlew checkstyle to ensure that everything is aligned to our code guidelines?

BTW: Android Studio doesn't pick the project's style settings. I fixed the indent so that running code format, the results don't look completely different, but there might still be some minor differences.

Checkstyle finishes successfully 👍.

Guardiola31337 · 2018-07-18T13:55:43Z

@boldtrn

Sure, not a problem, should I just squash it into 1 commit?

Yeah, that'd be 💯

BTW: Android Studio doesn't pick the project's style settings.

Yeah you need to setup 👀 https://github.com/mapbox/mapbox-navigation-android/blob/master/CONTRIBUTING.md#contributing. Although we need to update the wiki entry (link is currently broken 🤕). For now, you can 👀 mapbox-gl-native Android checkstyle guide https://github.com/mapbox/mapbox-gl-native/wiki/Setting-up-Mapbox-checkstyle
BTW good catch, thanks!

I fixed the indent so that running code format, the results don't look completely different, but there might still be some minor differences.

Checkstyle finishes successfully 👍.

Yeah, let's double check after squashing the commits and bringing your changes to a new branch, the CI will tell us 😛

Thank you for your patience here 🙏

boldtrn · 2018-07-18T14:03:36Z

Thanks, I squashed my commits.

Thank you for your patience here pray

No worries :)

Guardiola31337 · 2018-07-18T14:06:32Z

+    assertFalse(isUserOffRoute);
+  }
+
+  private void removeAllButOneStepPoints(RouteProgress routeProgress) {


NIT - Could you place this method at the bottom with the rest of the private methods?

Sure :).

BTW: I checked before first committing this method, there are no other private methods at the bottom ;).

I pointed it out because we normally organize the code in this particular order 👉 public, protected, private, etc. trying to locate them in order of calling.
Especially here (in tests) so we don't have private methods mixed with the tests 😉

Thanks for the clarification :).

Guardiola31337 · 2018-07-18T14:21:29Z

Hey @boldtrn 👋 last nit picks #1108 (comment) and #1108 (comment)

Promise!

Refactored NavigationMapRoute according to comments Removed final and extracted conditions to variables Show exception Fix exception Refactored OffRouteDetectorTest Refactored OffRouteDetectorTest according to comments

boldtrn · 2018-07-19T07:10:13Z

I updated the PR and squashed it again. Thanks for the comments @Guardiola31337. Let me know if this is alright or if I should change anything.

Guardiola31337 · 2018-07-19T10:32:46Z

Let me know if this is alright or if I should change anything.

Nah, everything is 💯
Following up on this PR in #1122

Thank you for the contribution @boldtrn
Really appreciate it 🙏

danesfeder reviewed Jul 12, 2018

View reviewed changes

danesfeder reviewed Jul 16, 2018

View reviewed changes

Guardiola31337 reviewed Jul 16, 2018

View reviewed changes

boldtrn commented Jul 17, 2018

View reviewed changes

Guardiola31337 reviewed Jul 17, 2018

View reviewed changes

boldtrn force-pushed the point_checks branch from f482fa0 to 25b3519 Compare July 18, 2018 14:02

Guardiola31337 reviewed Jul 18, 2018

View reviewed changes

Improved catching low point amounts

2d2f73d

Refactored NavigationMapRoute according to comments Removed final and extracted conditions to variables Show exception Fix exception Refactored OffRouteDetectorTest Refactored OffRouteDetectorTest according to comments

boldtrn force-pushed the point_checks branch from abcfb77 to 2d2f73d Compare July 19, 2018 07:08

Guardiola31337 mentioned this pull request Jul 19, 2018

Improve catching low point amounts #1122

Merged

Guardiola31337 closed this Jul 19, 2018

karussell mentioned this pull request Oct 4, 2018

License of closed source library libandroid-navigation #1391

Closed

Conversation

boldtrn commented Jul 12, 2018

Uh oh!

danesfeder left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

boldtrn commented Jul 15, 2018

Uh oh!

danesfeder left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Guardiola31337 left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

boldtrn commented Jul 16, 2018

Uh oh!

Guardiola31337 commented Jul 17, 2018

Uh oh!

Choose a reason for hiding this comment

Uh oh!

danesfeder commented Jul 17, 2018

Uh oh!

boldtrn commented Jul 17, 2018

Uh oh!

Guardiola31337 left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Guardiola31337 commented Jul 18, 2018

Uh oh!

boldtrn commented Jul 18, 2018

Uh oh!

Guardiola31337 commented Jul 18, 2018

Uh oh!

boldtrn commented Jul 18, 2018

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Guardiola31337 commented Jul 19, 2018 •

edited

Loading