WIP Issue 443 - Replace MKDirections with Google Distance Matrix API by fluffyemily · Pull Request #479 · mozilla-mobile/prox

fluffyemily · 2017-01-04T14:27:18Z

MKDirections demonstrated itself to be pretty crap during Hawaii. I started an experiment to replace it with the Google Distance Matrix API to see if it would be any better. I have left the MKDirections code in to make it easy to swap between them for comparison purposes.

There will almost certainly have to be some tidying up done here. Also, the API is rate limited as described the usage limits, so we would have to think about paying for the service if we were to take this implementation forward.

mcomella

lgtm (note: I didn't review the tests due to friday brain fog).

mcomella · 2017-01-06T20:09:53Z

 <?xml version="1.0" encoding="UTF-8"?>
 <Scheme
-   LastUpgradeVersion = "0800"
+   LastUpgradeVersion = "0820"


Not sure what this is – intended?

mcomella · 2017-01-06T20:11:39Z

 				CLANG_WARN_INFINITE_RECURSION = YES;
 				CLANG_WARN_INT_CONVERSION = YES;
 				CLANG_WARN_OBJC_ROOT_CLASS = YES_ERROR;
+				CLANG_WARN_SUSPICIOUS_MOVE = YES;


mcomella · 2017-01-06T20:11:44Z

 			baseConfigurationReference = 1E318DF6890B9E7221222152 /* Pods-Prox.enterprise kona.xcconfig */;
 			buildSettings = {
-				ALWAYS_EMBED_SWIFT_STANDARD_LIBRARIES = "$(inherited)";
+				ALWAYS_EMBED_SWIFT_STANDARD_LIBRARIES = YES;


mcomella · 2017-01-06T20:11:59Z

 				PRODUCT_BUNDLE_IDENTIFIER = com.mozilla.Prox;
 				PRODUCT_NAME = "$(TARGET_NAME)";
 				SWIFT_OBJC_BRIDGING_HEADER = "$(PROJECT_DIR)/Prox/Prox-Bridging-Header.h";
+				SWIFT_OPTIMIZATION_LEVEL = "-Onone";


mcomella · 2017-01-06T20:22:36Z

+var MIN_WALKING_TIME: Int = {
+    // Note that this is semantically maximum walking time,
+    // rather than minimum walking time (as used throughout the codebase).
+    return RemoteConfigKeys.maxWalkingTimeInMins.value


nit: I don't really understand the min vs. max idea – can you clarify the comment?

I didn't add this comment, only copied it from inside MKDirectionsTravelTimesProvider

mcomella · 2017-01-07T00:33:43Z

-            for (index, place) in sortedByTravelTime.enumerated() {
-                sortedPlaces[index] = place
+            let sortedByTravelTime: [Place] = sortedTravelTimes.flatMap { time in
+                let placeIndex = sortedByDistance.index { place in


As in the other PR, I think Array.index could be linear search and since we're iterating over the places, this could be n² run time.

mcomella · 2017-01-07T00:38:00Z

+            let sortedByTravelTime: [Place] = sortedTravelTimes.flatMap { time in
+                let placeIndex = sortedByDistance.index { place in
+                    return time.destinationPlaceKey == place.id
+                        || (time.destination?.latitude == place.latLong.latitude && time.destination?.longitude == place.latLong.longitude)


What are we checking here (with the coordinates)? Can you add a comment or store in a named temporary variable to clarify?

We cannot guarentee that every TravelTime returned will have a place id (it depends on which function we used to fetch the TravelTime. Therefore if the TravelTime has no place id, we have to use the TravelTime destination coordinates to match times with places instead.

mcomella · 2017-01-07T00:39:51Z

-
-            for (index, place) in sortedByTravelTime.enumerated() {
-                sortedPlaces[index] = place
+            let sortedByTravelTime: [Place] = sortedTravelTimes.flatMap { time in


nit: I think there's stronger intent when using filter to remove items from a collection than flatMap.

But we are also mapping TravelTimes to Place's as well as filtering out times that we cannot map to places so flatmap prevents us from having to filter and then map which would increase the number of interations

mcomella · 2017-01-07T00:47:00Z

-            guard let travelTimes = result.successResult(),
-            !travelTimes.isEmpty else { return completion(sortedByDistance) }
+            guard var travelTimes = result.successResult() else { return completion(sortedByDistance) }
+            travelTimes += placesWithTimes.flatMap { $0.cachedTravelTime(fromLocation: location) }


nit: I think flatMap is redundant – the placesWithTimes collection is created with the flatMap function.

mcomella · 2017-01-07T00:49:18Z

                }
                if placeIndex != nil {
                    let place = sortedByDistance[placeIndex!]
+                    Place.travelTimesCache[place.id] = CachedTravelTime(Deferred(filledWith: DatabaseResult.succeed(value: time)), location)


flatMap comes from a functional background so it's a little unexpected for it to have side effects but if this is what is simplest, so be it.

…atrix

…ding to convention in the rest of the app

…king time

fluffyemily self-assigned this Jan 4, 2017

fluffyemily requested review from jhugman and mcomella January 4, 2017 14:27

fluffyemily added the travel times label Jan 4, 2017

mcomella approved these changes Jan 7, 2017

View reviewed changes

Emily Toop added 12 commits January 9, 2017 15:53

Google directions

b38a926

ttp

652ff07

Simple apples for apples replacements of MKDirections with Distance M…

ecc2e20

…atrix

Issue 443 - Fetch Google travel times in batches

640b0a1

Issue 443 - use cached travel times for a location where they exist

02f62f3

Issue 443 - Update tests to run on all travel times provider types

6f342a3

Issue 443 - implement multi location fetch in MKDirections

71de3df

Issue 443 - Replace Strings for Place.id and Event.id with aliased types

0ac2ebe

Issue 443 - move travel time constants to AppConstants and name accor…

b67696e

…ding to convention in the rest of the app

Issue 443 - make variable clearer to read around driving time and wal…

6ba92e3

…king time

Issue 443 - Calculate expectedResponseCount in advance

ea803e4

Issue 443 - Addressing Review Comments

ff97d93

fluffyemily force-pushed the fluffyemily/issue-443-google-distance-matrix branch from bfae309 to ff97d93 Compare January 10, 2017 13:16

Disable travel time tests for ProxEnterprise

31b1b9d

Conversation

fluffyemily commented Jan 4, 2017 • edited by mcomella Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

mcomella 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!

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!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

fluffyemily commented Jan 4, 2017 •

edited by mcomella

Loading