WIP Issue 443 - Replace MKDirections with Google Distance Matrix API #479
Conversation
| @@ -1,6 +1,6 @@ | |||
| <?xml version="1.0" encoding="UTF-8"?> | |||
| <Scheme | |||
| LastUpgradeVersion = "0800" | |||
| LastUpgradeVersion = "0820" | |||
There was a problem hiding this comment.
Not sure what this is – intended?
| @@ -942,6 +948,7 @@ | |||
| CLANG_WARN_INFINITE_RECURSION = YES; | |||
| CLANG_WARN_INT_CONVERSION = YES; | |||
| CLANG_WARN_OBJC_ROOT_CLASS = YES_ERROR; | |||
| CLANG_WARN_SUSPICIOUS_MOVE = YES; | |||
| @@ -971,7 +978,7 @@ | |||
| isa = XCBuildConfiguration; | |||
| baseConfigurationReference = 1E318DF6890B9E7221222152 /* Pods-Prox.enterprise kona.xcconfig */; | |||
| buildSettings = { | |||
| ALWAYS_EMBED_SWIFT_STANDARD_LIBRARIES = "$(inherited)"; | |||
| ALWAYS_EMBED_SWIFT_STANDARD_LIBRARIES = YES; | |||
| @@ -1265,6 +1275,7 @@ | |||
| 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"; | |||
| 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 |
There was a problem hiding this comment.
nit: I don't really understand the min vs. max idea – can you clarify the comment?
There was a problem hiding this comment.
I didn't add this comment, only copied it from inside MKDirectionsTravelTimesProvider
| for (index, place) in sortedByTravelTime.enumerated() { | ||
| sortedPlaces[index] = place | ||
| let sortedByTravelTime: [Place] = sortedTravelTimes.flatMap { time in | ||
| let placeIndex = sortedByDistance.index { place in |
There was a problem hiding this comment.
As in the other PR, I think Array.index could be linear search and since we're iterating over the places, this could be n2 run time.
| 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) |
There was a problem hiding this comment.
What are we checking here (with the coordinates)? Can you add a comment or store in a named temporary variable to clarify?
There was a problem hiding this comment.
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.
|
|
||
| for (index, place) in sortedByTravelTime.enumerated() { | ||
| sortedPlaces[index] = place | ||
| let sortedByTravelTime: [Place] = sortedTravelTimes.flatMap { time in |
There was a problem hiding this comment.
nit: I think there's stronger intent when using filter to remove items from a collection than flatMap.
There was a problem hiding this comment.
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
| 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) } |
There was a problem hiding this comment.
nit: I think flatMap is redundant – the placesWithTimes collection is created with the flatMap function.
| @@ -48,6 +50,7 @@ struct PlaceUtilities { | |||
| } | |||
| if placeIndex != nil { | |||
| let place = sortedByDistance[placeIndex!] | |||
| Place.travelTimesCache[place.id] = CachedTravelTime(Deferred(filledWith: DatabaseResult.succeed(value: time)), location) | |||
There was a problem hiding this comment.
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.
…ding to convention in the rest of the app
fluffyemily
force-pushed
the
fluffyemily/issue-443-google-distance-matrix
branch
from
bfae309 to
ff97d93
Compare
#443
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.