Cache route options / calculate remaining waypoints #680

danesfeder · 2018-01-29T17:51:26Z

Closes #609

danesfeder · 2018-02-12T18:57:27Z

@devotaaabel @Guardiola31337 this is also good for 👀

devotaaabel · 2018-02-13T19:28:44Z

...tion-ui/src/main/java/com/mapbox/services/android/navigation/ui/v5/route/RouteViewModel.java

+    }
+  }
+
+  private void cacheRouteOptions(RouteOptions routeOptions) {


Does this really need it's own method? I'm not sure cacheRouteDestination() fits under the name of this method either

What about encapsulating all cache-related calls included in extractRouteFromOptions method

mapbox-navigation-android/libandroid-navigation-ui/src/main/java/com/mapbox/services/android/navigation/ui/v5/route/RouteViewModel.java

Lines 159 to 161 in cb1af2f

cacheRouteOptions(route.routeOptions());

cacheRouteProfile(options);

cacheRouteLanguage(options, route);

into a cache method and fixing the naming of these methods (concrete names) so that they reflect clearer what they're doing in this context?

devotaaabel · 2018-02-13T19:31:05Z

...tion-ui/src/main/java/com/mapbox/services/android/navigation/ui/v5/route/RouteViewModel.java

-  private void cacheRouteProfile(NavigationViewOptions options, DirectionsRoute route) {
-    String profile = options.directionsProfile();
-    routeProfile = profile != null ? profile : route.routeOptions().profile();
+  private void cacheRouteProfile(NavigationViewOptions options) {


I'm not sure this needs it's own method either since the logic has been removed

Don't see why not if it makes the intention clearer and the code easy to read and understand.

devotaaabel · 2018-02-13T19:35:04Z

...tion-ui/src/main/java/com/mapbox/services/android/navigation/ui/v5/route/RouteViewModel.java

@@ -240,4 +234,21 @@ private boolean validRouteResponse(Response<DirectionsResponse> response) {
    return response.body() != null
      && !response.body().routes().isEmpty();
  }
+
+  private void addWaypoints(List<Point> remainingCoordinates, NavigationRoute.Builder builder) {
+    if (!remainingCoordinates.isEmpty()) {


isEmpty check is redundant

Yeah we're already checking that right before calling addWaypoints 👀

mapbox-navigation-android/libandroid-navigation-ui/src/main/java/com/mapbox/services/android/navigation/ui/v5/route/RouteViewModel.java

Lines 117 to 120 in cb1af2f

if (!remainingWaypoints.isEmpty()) {

builder.destination(remainingWaypoints.remove(remainingWaypoints.size() - 1));

addWaypoints(remainingWaypoints, builder);

}

We should remove from either here or fetchRouteFromOffRouteEvent, the latter preferable (making public fetchRouteFromOffRouteEvent method cleaner).

devotaaabel · 2018-02-13T19:50:05Z

...id-navigation/src/main/java/com/mapbox/services/android/navigation/v5/route/RouteEngine.java

@@ -35,15 +36,15 @@ public void fetchRoute(Point origin, RouteProgress routeProgress) {
    this.routeProgress = routeProgress;

    // Calculate remaining waypoints
-    List<Point> coordinates = new ArrayList<>(routeProgress.directionsRoute().routeOptions().coordinates());


This method seems really similar to the one in RouteViewModel, maybe some of it could be extracted into NavigationRoute? Or maybe somewhere else

Yeah definitely a lot of duplicate here, good call

devotaaabel · 2018-02-13T19:52:18Z

...oid-navigation/src/main/java/com/mapbox/services/android/navigation/v5/utils/RouteUtils.java

@@ -74,14 +76,36 @@ public static boolean isLastLeg(RouteProgress routeProgress) {
    return currentLeg.equals(legs.get(legs.size() - 1));
  }

+  /**
+   * Given a {@link RouteProgress}, this method will calculate the remaining coordinates
+   * along the given route based on total route coordinates and the progress remaining waypoints.


Add to javadoc cases that it returns null

Guardiola31337

👀 good opportunity to apply The Boy Scout Rule
Let's do it 😉

Guardiola31337 · 2018-02-15T11:33:41Z

...-ui/src/main/java/com/mapbox/services/android/navigation/ui/v5/NavigationViewSubscriber.java

-        if (newOrigin != null) {
+      public void onChanged(@Nullable OffRouteEvent offRouteEvent) {
+        if (offRouteEvent != null) {
+          Point newOrigin = offRouteEvent.getNewOrigin();
          if (navigationViewEventDispatcher.allowRerouteFrom(newOrigin)) {
            // Send off route event with new origin


This comment is not adding any value, it should be removed.
IMO looking at navigationViewEventDispatcher.onOffRoute(newOrigin) you know what's going on making the comment superfluous and not needed.

Guardiola31337 · 2018-02-15T11:38:57Z

...-ui/src/main/java/com/mapbox/services/android/navigation/ui/v5/NavigationViewSubscriber.java

-        if (newOrigin != null) {
+      public void onChanged(@Nullable OffRouteEvent offRouteEvent) {
+        if (offRouteEvent != null) {
+          Point newOrigin = offRouteEvent.getNewOrigin();
          if (navigationViewEventDispatcher.allowRerouteFrom(newOrigin)) {
            // Send off route event with new origin
            navigationViewEventDispatcher.onOffRoute(newOrigin);
            // Fetch a new route with the given origin


If you update the functionality but not the comment, the comment becomes out of date and it starts lying. This is an example of why I'm not a big fan of comments

Guardiola31337 · 2018-02-15T11:51:55Z

...-ui/src/main/java/com/mapbox/services/android/navigation/ui/v5/NavigationViewSubscriber.java

          if (navigationViewEventDispatcher.allowRerouteFrom(newOrigin)) {
            // Send off route event with new origin
            navigationViewEventDispatcher.onOffRoute(newOrigin);
            // Fetch a new route with the given origin
-            routeViewModel.fetchRouteNewOrigin(newOrigin);
+            routeViewModel.fetchRouteFromOffRouteEvent(offRouteEvent);
            // To prevent from firing on rotation


In order to make the intention clearer, what about extracting this call into a private method and give it a name based on the comment? This way onChanged will be easier to read and understand and comment won't be necessary + anyone who looks at the code will know what's going on at a glance.

Guardiola31337 · 2018-02-15T11:56:38Z

...tion-ui/src/main/java/com/mapbox/services/android/navigation/ui/v5/route/RouteViewModel.java

-  private void fetchRoute(Point origin, Point destination) {
-    if (origin != null && destination != null) {
-
+  public void fetchRouteFromOffRouteEvent(OffRouteEvent offRouteEvent) {


IMO fetchRouteFromOffRouteEvent is getting too long.
What about extracting blocks of code into well-named (based on the comments) private methods and simplifying conditional expressions?

NIT OffRouteEvent offRouteEvent is kind of repetitive. Calling it just event should be fine, IMO it's clear based on the context that it's an OffRouteEvent.

Guardiola31337 · 2018-02-15T11:58:55Z

...tion-ui/src/main/java/com/mapbox/services/android/navigation/ui/v5/route/RouteViewModel.java

+      .accessToken(Mapbox.getAccessToken())
+      .origin(origin)
+      .destination(destination);
+    // Add any overridden values based on NavigationViewOptions


This comment is not adding any value, it should be removed.
IMO looking at addNavigationViewOptions(builder) you know what's going on making the comment superfluous and not needed.

Guardiola31337 · 2018-02-15T12:58:46Z

...tion-ui/src/main/java/com/mapbox/services/android/navigation/ui/v5/route/RouteViewModel.java

@@ -240,4 +234,21 @@ private boolean validRouteResponse(Response<DirectionsResponse> response) {
    return response.body() != null
      && !response.body().routes().isEmpty();
  }
+
+  private void addWaypoints(List<Point> remainingCoordinates, NavigationRoute.Builder builder) {
+    if (!remainingCoordinates.isEmpty()) {


Yeah we're already checking that right before calling addWaypoints 👀

mapbox-navigation-android/libandroid-navigation-ui/src/main/java/com/mapbox/services/android/navigation/ui/v5/route/RouteViewModel.java

Lines 117 to 120 in cb1af2f

if (!remainingWaypoints.isEmpty()) {

builder.destination(remainingWaypoints.remove(remainingWaypoints.size() - 1));

addWaypoints(remainingWaypoints, builder);

}

We should remove from either here or fetchRouteFromOffRouteEvent, the latter preferable (making public fetchRouteFromOffRouteEvent method cleaner).

Guardiola31337 · 2018-02-15T13:01:03Z

...id-navigation/src/main/java/com/mapbox/services/android/navigation/v5/route/RouteEngine.java

@@ -35,15 +36,15 @@ public void fetchRoute(Point origin, RouteProgress routeProgress) {
    this.routeProgress = routeProgress;

    // Calculate remaining waypoints


This comment is not adding any value, it should be removed.

Guardiola31337 · 2018-02-15T13:02:46Z

...id-navigation/src/main/java/com/mapbox/services/android/navigation/v5/route/RouteEngine.java

      return;
    }
-    // Remove any waypoints that have been passed
-    coordinates.subList(0, routeProgress.remainingWaypoints()).clear();
+
    // Get the destination waypoint (last in the list)


Could we extract waypoints.size() - 1 into an explaining variable called last and remove the comment?

Also it's kinda confusing that calling remove returns the element removed so maybe it's preferable to extract waypoints.remove(waypoints.size() - 1) into a method reflecting that. What do you think?

Guardiola31337 · 2018-02-15T13:10:24Z

...id-navigation/src/main/java/com/mapbox/services/android/navigation/v5/route/RouteEngine.java

@@ -52,7 +53,7 @@ public void fetchRoute(Point origin, RouteProgress routeProgress) {
      .routeOptions(currentOptions);

    // Add waypoints with the remaining coordinate values


What about fixing waypoints

mapbox-navigation-android/libandroid-navigation/src/main/java/com/mapbox/services/android/navigation/v5/route/RouteEngine.java

Line 39 in cb1af2f

List<Point> waypoints = RouteUtils.calculateRemainingWaypoints(routeProgress);

name into something like remaining and remove both comments (here and L38)?

Guardiola31337 · 2018-02-15T13:13:19Z

...oid-navigation/src/main/java/com/mapbox/services/android/navigation/v5/utils/RouteUtils.java

+    if (coordinates.size() < routeProgress.remainingWaypoints()) {
+      return null;
+    }
+    // Remove any waypoints that have been passed


Is this comment needed?

Guardiola31337

Good job refactoring the code 🙇

There is still room for improvement (minor comments below) but definitely not blocking the PR.

Guardiola31337 · 2018-02-16T12:54:35Z

...-ui/src/main/java/com/mapbox/services/android/navigation/ui/v5/NavigationViewSubscriber.java

      @Override
-      public void onChanged(@Nullable Point newOrigin) {
-        if (newOrigin != null) {
+      public void onChanged(@Nullable OffRouteEvent offRouteEvent) {


NIT OffRouteEvent offRouteEvent is kind of repetitive. Calling it just event should be fine, IMO it's clear based on the context that it's an OffRouteEvent.

Guardiola31337 · 2018-02-16T12:56:16Z

...tion-ui/src/main/java/com/mapbox/services/android/navigation/ui/v5/route/RouteViewModel.java

-  private void fetchRoute(Point origin, Point destination) {
-    if (origin != null && destination != null) {
-
+  public void fetchRouteFromOffRouteEvent(OffRouteEvent offRouteEvent) {


NIT OffRouteEvent offRouteEvent is kind of repetitive. Calling it just event should be fine, IMO it's clear based on the context that it's an OffRouteEvent.

Guardiola31337 · 2018-02-16T12:58:35Z

.../src/test/java/com/mapbox/services/android/navigation/v5/navigation/NavigationRouteTest.java

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


NIT Empty line

Guardiola31337 · 2018-02-16T12:59:26Z

...id-navigation/src/main/java/com/mapbox/services/android/navigation/v5/route/RouteEngine.java

+                                                                             RouteProgress progress) {
+    RouteOptions options = progress.directionsRoute().routeOptions();
+    NavigationRoute.Builder builder = NavigationRoute.builder()
+      .origin(origin, bearing, 90d)


Magic number

Guardiola31337 · 2018-02-16T12:59:39Z

...id-navigation/src/main/java/com/mapbox/services/android/navigation/v5/route/RouteEngine.java

+  }
+
+  private static Point retrieveDestinationWaypoint(List<Point> remainingWaypoints) {
+    int lastWaypoint = remainingWaypoints.size() - 1;


Magic number

Guardiola31337 · 2018-02-16T12:59:54Z

...tion-ui/src/main/java/com/mapbox/services/android/navigation/ui/v5/route/RouteViewModel.java

+  private void cacheRouteDestination() {
+    if (routeOptions != null && !routeOptions.coordinates().isEmpty()) {
+      List<Point> coordinates = routeOptions.coordinates();
+      Point destinationPoint = coordinates.get(coordinates.size() - 1);


Magic number

Cache route options / calculate remaining waypoints

dc2bda4

danesfeder added the bug Defect to be fixed. label Jan 29, 2018

danesfeder added this to the v0.10.0 milestone Jan 29, 2018

danesfeder self-assigned this Jan 29, 2018

danesfeder requested review from devotaaabel and Guardiola31337 January 29, 2018 17:51

danesfeder added 3 commits February 8, 2018 17:00

Merge branch 'master' into dan-dropped-waypoints

4831a94

Merge branch 'master' into dan-dropped-waypoints

f2ce3fd

Clean up and add route options test

cb1af2f

danesfeder added the ✓ ready for review label Feb 12, 2018

devotaaabel reviewed Feb 13, 2018

View reviewed changes

Guardiola31337 requested changes Feb 15, 2018

View reviewed changes

danesfeder added 3 commits February 15, 2018 15:03

Fixes / cleanup per review

796d83b

Merge branch 'master' into dan-dropped-waypoints

f8ccbe3

Merge branch 'master' into dan-dropped-waypoints

a17e521

Guardiola31337 approved these changes Feb 16, 2018

View reviewed changes

danesfeder added 3 commits February 16, 2018 11:55

Clean up per review

bb9660c

Merge branch 'master' into dan-dropped-waypoints

9808843

Nit extra line

269c999

danesfeder merged commit 48f2bf0 into master Feb 16, 2018

danesfeder deleted the dan-dropped-waypoints branch February 16, 2018 17:27

danesfeder mentioned this pull request Feb 26, 2018

Release v0.10.0 #722

Closed

13 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Cache route options / calculate remaining waypoints #680

Cache route options / calculate remaining waypoints #680

danesfeder commented Jan 29, 2018

danesfeder commented Feb 12, 2018

devotaaabel Feb 13, 2018

Guardiola31337 Feb 15, 2018

devotaaabel Feb 13, 2018

Guardiola31337 Feb 15, 2018

devotaaabel Feb 13, 2018

Guardiola31337 Feb 15, 2018

devotaaabel Feb 13, 2018

danesfeder Feb 15, 2018

devotaaabel Feb 13, 2018

Guardiola31337 left a comment

Guardiola31337 Feb 15, 2018

Guardiola31337 Feb 15, 2018

Guardiola31337 Feb 15, 2018

Guardiola31337 Feb 15, 2018

Guardiola31337 Feb 16, 2018

Guardiola31337 Feb 15, 2018

Guardiola31337 Feb 15, 2018

Guardiola31337 Feb 15, 2018

Guardiola31337 Feb 15, 2018

Guardiola31337 Feb 15, 2018

Guardiola31337 Feb 15, 2018

Guardiola31337 Feb 15, 2018

Guardiola31337 left a comment

Guardiola31337 Feb 16, 2018

Guardiola31337 Feb 16, 2018

Guardiola31337 Feb 16, 2018

Guardiola31337 Feb 16, 2018

Guardiola31337 Feb 16, 2018

Guardiola31337 Feb 16, 2018

	cacheRouteOptions(route.routeOptions());
	cacheRouteProfile(options);
	cacheRouteLanguage(options, route);

	if (!remainingWaypoints.isEmpty()) {
	builder.destination(remainingWaypoints.remove(remainingWaypoints.size() - 1));
	addWaypoints(remainingWaypoints, builder);
	}

		@@ -35,15 +36,15 @@ public void fetchRoute(Point origin, RouteProgress routeProgress) {
		this.routeProgress = routeProgress;

		// Calculate remaining waypoints

		@@ -52,7 +53,7 @@ public void fetchRoute(Point origin, RouteProgress routeProgress) {
		.routeOptions(currentOptions);

		// Add waypoints with the remaining coordinate values


		@Test
		public void addRouteOptionsIncludedInRequest() throws Exception {

Cache route options / calculate remaining waypoints #680

Cache route options / calculate remaining waypoints #680

Conversation

danesfeder commented Jan 29, 2018

danesfeder commented Feb 12, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Guardiola31337 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Guardiola31337 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment