Recursive function to show directions for multiple locations

Question

I have a Mapview which shows a bunch of locations (Events). Here's the code of an Event object.

class Event: NSObject {
 var latitude: CLLocationDegrees!
 var longitude: CLLocationDegrees!
 var coordinates: CLLocationCoordinate2D {
 return CLLocationCoordinate2D(latitude: latitude, longitude: longitude)
 }
 override init() {
 super.init()
 }
 init(latitude: CLLocationDegrees, longitude: CLLocationDegrees) {
 self.latitude = latitude
 self.longitude = longitude
 }
}

enter image description here

I need to show directions to each of these locations and the directions should be calculated based on which location is the closest next. For example, if you start from the user's current location, it checks which location is the nearest and draws the route to that location. Then from there, it checks which is the next nearest one and so on.

I was able to accomplish this from the below code (I removed the unnecessary methods to make it shorter. I've attached a working example at the end).

class ViewController: UIViewController {
 @IBOutlet weak var mapView: MKMapView!
 private let locationManager = CLLocationManager()
 private var events = [Event]()
 private var coordinates = [CLLocationCoordinate2D]()
 private func getDirections(#fromLocationCoord: CLLocationCoordinate2D, toLocationCoord: CLLocationCoordinate2D) {
 let fromLocationMapItem = MKMapItem(placemark: MKPlacemark(coordinate: fromLocationCoord, addressDictionary: nil))
 let toLocationMapItem = MKMapItem(placemark: MKPlacemark(coordinate: toLocationCoord, addressDictionary: nil))
 let directionsRequest = MKDirectionsRequest()
 directionsRequest.transportType = .Automobile
 directionsRequest.setSource(fromLocationMapItem)
 directionsRequest.setDestination(toLocationMapItem)
 let directions = MKDirections(request: directionsRequest)
 directions.calculateDirectionsWithCompletionHandler { (directionsResponse, error) -> Void in
 if let error = error {
 println("Error getting directions: \(error.localizedDescription)")
 } else {
 let route = directionsResponse.routes[0] as! MKRoute
 // draw the route in the map
 self.mapView.addOverlay(route.polyline)
 // get the next closest location
 let closestLocation = self.getClosestLocation(toLocationCoord, locations: self.coordinates)
 if let closest = closestLocation {
 self.getDirections(fromLocationCoord: toLocationCoord, toLocationCoord: closest)
 }
 // remove the current location's coordinates from the array
 self.coordinates = self.coordinates.filter({ 0ドル != toLocationCoord })
 }
 }
 }
 private func getClosestLocation(location: CLLocationCoordinate2D, locations: [CLLocationCoordinate2D]) -> CLLocationCoordinate2D? {
 var closestLocation: (distance: Double, coordinates: CLLocationCoordinate2D)?
 for loc in locations {
 let distance = round(location.location.distanceFromLocation(loc.location)) as Double
 if closestLocation == nil {
 closestLocation = (distance, loc)
 } else {
 if distance < closestLocation!.distance {
 closestLocation = (distance, loc)
 }
 }
 }
 return closestLocation?.coordinates
 }
}
// MARK: - MKMapViewDelegate
extension ViewController: MKMapViewDelegate {
 func mapView(mapView: MKMapView!, rendererForOverlay overlay: MKOverlay!) -> MKOverlayRenderer! {
 let renderer = MKPolylineRenderer(overlay: overlay)
 renderer.strokeColor = UIColor.blueColor()
 renderer.lineWidth = 5
 return renderer
 }
 func mapView(mapView: MKMapView!, didUpdateUserLocation userLocation: MKUserLocation!) {
 mapView.removeOverlays(mapView.overlays)
 coordinates = events.map({ 0ドル.coordinates })
 let closestLocation = getClosestLocation(userLocation.coordinate, locations: coordinates)
 if let closest = closestLocation {
 getDirections(fromLocationCoord: userLocation.coordinate, toLocationCoord: closest)
 }
 }
}

I'll briefly explain what the code is doing. After the map is loaded and when the map detects the user's current location in the didUpdateUserLocation method, it first collects the coordinates of all events into an array called coordinates. Then it fires off the getClosestLocation method. Once it fetches the closest location's coordinates, they're passed to the getDirections get get the route to that location. After the route is received, I add its overlay which draws the route on the map. Then once again it does this same process over again in a recursive manner.

This works as I want.

enter image description here

But I'm not too happy with the code. It's seems inefficient and lacks elegance. I did what I know to shorten the code by applying some of the Swift features like operator overloading, extensions.

extension CLLocationCoordinate2D {
 var location: CLLocation {
 return CLLocation(latitude: latitude, longitude: longitude)
 }
}
func ==(lhs: CLLocationCoordinate2D, rhs: CLLocationCoordinate2D) -> Bool {
 if lhs.latitude == rhs.latitude && lhs.longitude == rhs.longitude {
 return true
 }
 return false
}
func !=(lhs: CLLocationCoordinate2D, rhs: CLLocationCoordinate2D) -> Bool {
 return !(lhs == rhs)
}

But still I think there is room for improvement.

Can anyone please tell me where I can improve this code?

I uploaded a demo project to Dropbox if you wanna take a look at it.

Answer 1

private func getDirections(#fromLocationCoord: CLLocationCoordinate2D, toLocationCoord: CLLocationCoordinate2D) {
 let fromLocationMapItem = MKMapItem(placemark: MKPlacemark(coordinate: fromLocationCoord, addressDictionary: nil))
 let toLocationMapItem = MKMapItem(placemark: MKPlacemark(coordinate: toLocationCoord, addressDictionary: nil))
 let directionsRequest = MKDirectionsRequest()
 directionsRequest.transportType = .Automobile
 directionsRequest.setSource(fromLocationMapItem)
 directionsRequest.setDestination(toLocationMapItem)
 let directions = MKDirections(request: directionsRequest)
 directions.calculateDirectionsWithCompletionHandler { (directionsResponse, error) -> Void in
 if let error = error {
 println("Error getting directions: \(error.localizedDescription)")
 } else {
 let route = directionsResponse.routes[0] as! MKRoute
 // draw the route in the map
 self.mapView.addOverlay(route.polyline)
 // get the next closest location
 let closestLocation = self.getClosestLocation(toLocationCoord, locations: self.coordinates)
 if let closest = closestLocation {
 self.getDirections(fromLocationCoord: toLocationCoord, toLocationCoord: closest)
 }
 // remove the current location's coordinates from the array
 self.coordinates = self.coordinates.filter({ 0ドル != toLocationCoord })
 }
 }
}
private func getClosestLocation(location: CLLocationCoordinate2D, locations: [CLLocationCoordinate2D]) -> CLLocationCoordinate2D? {
 var closestLocation: (distance: Double, coordinates: CLLocationCoordinate2D)?
 for loc in locations {
 let distance = round(location.location.distanceFromLocation(loc.location)) as Double
 if closestLocation == nil {
 closestLocation = (distance, loc)
 } else {
 if distance < closestLocation!.distance {
 closestLocation = (distance, loc)
 }
 }
 }
 return closestLocation?.coordinates
}

I'll start with the bottom method here. I like what this method does. Using the tuple helps keep the code a bit more organized. I'd only argue that the distance's type should be CLLocationDistance rather than a Double. Yes, CLLocationDistance is effectively just a typealias for Double... but Apple could change it. Plus this is Swift. Our types should match correctly.

What I don't like about this bottom method so much is the naming. Instead of making the method name quite readable (keeping in mind that the part inside the parenthesis is part of the name), you've just given the method an identifier and the variables some names.

Let's try this on for size instead:

private func closestLocation(locations:[CLLocationCoordinate2D], toLocation: CLLocationCoordinate2D) -> CLLocationCoorindate2D?

It seems minor, but it's a slight readability improvement that can help ease the understanding of future maintainers.

And arguably, this method shouldn't even take two arguments. How about a CLLocationCoordinate2D extension?

extension CLLocationCoorindate2D {
 func closestLocation(locations:[CLLocationCoordinate2D]) -> CLLocationCoordinate2D? {
 // calculate closest location to self and return it
 }
}

I think this would be the right approach. Later in this answer, I'll be making the case that these methods don't belong in your view controller.

---

Now, for the getDirections method...

Here's the thing... this method does too much. The biggest piece of evidence pointing us to that conclusion is that it's a "get" method that doesn't return anything.

But what is this method actually doing?

1. Gets the directions for the first pair of locations.
2. Draws the directions on the map.
3. Find the next pair of locations.
4. Recursively calls itself.
5. Shrinks the array of locations to visit.

Following the single-responsibility-principle means that a method shouldn't need a list of things it does. It should just do one thing.

Importantly, we have no possible way of really reusing this in any other scenario, even though it seems a highly reusable thing we're doing (getting a set of directions across an array of locations).

So... let's fix that.

In order to make this code reusable, we're going to need to refactor to separate the closest location logic from the directions logic. And keeping in mind that those two things can be done on a background thread, but drawing on the map can't, we'll refactor this into a separate method as well. Let's also keep in mind that those first two tasks (closest location, getting directions) don't belong in a view controller class.

I'm not going to write all of the code here... instead, I'll create an outline of how I'd reorganize the code. I think it'll be pretty easy to figure out from there.

extension CLLocationCoordinate2D {
 func closestLocation(locations:[CLLocationCoordinate2D]) -> CLLocationCoordinate2D? {
 // find closest location to self and return it
 }
 func locationsSortedByDistanceFromPreviousLocation(locations: [CLLocationCoordinate2D]) -> [CLLocationCoordinate2D] {
 // take in an array and a starting location
 // using the previous closestLocation function, build a distance-sorted array
 // return that array
 }
 func calculateDirections(toLocation: CLLocationCoordinate2D, transportType: MKDirectionsTransportType, completionHandler: MKDirectionsHandler) {
 // this is really just a convenience wrapper for doing MapKit calculation of directions
 }
 static calculateDirections(locations: [CLLocationCoordinate2D], transportType: MKDirectionsTransportType, completionHandler: ([MKRoute]) -> Void) {
 // one by one, iterate through the locations in the array
 // calculate their directions and add the route to an array
 // once you've got all the routes in an array and made it through the whole array, call the completion handler passing in the array of *all* the MKRoutes for every point
 }
}

So now we've added this methods in out CLLocationCoordinate2D extension. Using them will look something like this in our view controller:

let sortedDirections = startLocation. locationsSortedByDistanceFromPreviousLocation(yourLocationArray)
CLLocationCoordinate2D.calculateDirection(sortedDirections, transportType: .Automobile) { routes in
 // call method which adds the array of routes to the map
}

Important to keep in mind is that we can dump all of these calculations on to a background thread. These can easily take a long time, and as the size of the locations in the array that you're sorting and calculating directions for grows, the time it takes will grow too. So we really want this stuff on the background. Let the user do other things, keep the UI interactive etc.

• Isuru: Thanks a lot again. I'm in the process of refactoring my code according to your suggestions. I have a question. How do I utilize the two calculateDirections methods again? Why are there two of them?
• nhgrif: One of them takes a single location and calculates directions between self and the passed in location (from Point-A to Point-B). The other takes an array of locations and uses the first one between all of them to build a total of array of directions from Point-A to Point-B to Point-C to Point-D to Point-E, etc. ad naseum, for each point in the array.
• Isuru: I see. And also in the locationsSortedByDistanceFromPreviousLocation method, do you mean the distances between user's current location and event locations event (As in user location -> event location 1, user location -> event location 2) or distances between each event location is sequence (user location -> event location 1, event location 1 -> event location 2 so on)?
• nhgrif: It should do what you need it to do, which seems to be user -> loc1, loc1 -> loc2, etc.

Answer 2

This probably isn't exactly what you want reviews to focus on... but my first comments must be on your Event class.

There are numerous problems here. The most egregious of which is:

What does the Event class do that the CLLocationCoordinate2D struct does not already do?

The answer is nothing.

In fact, the differences are very few. Your class unsafely uses implicitly unwrapped optionals for the latitude and longitude, while CLLocationCoordinate2D does not.

Both your class and the CLLocationCoordinate2D struct offer the same constructors. The difference is that the CLLocationCoordinate2D sets initial values for latitude and longitude for the zero-argument constructor (0, 0), allowing it to use non-optionals for latitude and longitude.

The only other difference between your class and the built-in struct is only necessary because your Event class is not a CLLocationCoordinate2D. The coordinates variables... first of all, being plural, makes me think I'm getting an array... so it should be non-plural... but it simply assumes our latitude/longitude are non-nil (which it can't safely do) and then constructs a CLLocationCoordinate2D to pass back to us (since that's what we actually need for doing map work).

And the last difference is that your class is a class where the built-in type is a struct. Did you make that decision consciously? Is there a reason you need a class versus a struct?

---

Now, I can easily presume that the Event class is incomplete and will eventually have other information, perhaps an event description? List of attendees? Start time & stop time? I don't know.

If that presumption is correct, then your efforts at the class so far have been decapsulating data, which we really shouldn't do.

So, here's what I'd do...

For now, we can simply create a typealias for the CLLocationCoordinate2D struct:

typealias EventLocation = CLLocationCoordinate2D

So now, we can use the CLLocationCoordinate2D, but use it with a name that matches what we're using it for. Then, if we ever write that Event class, we just add EventLocation as a property of the class (and maintain proper encapsulation).

And there's actually good reason for typealias-ing versus just using the CLLocationCoordinate2D. If you want to use CLLocationCoordinate2D, you'll have to import CoreLocation in every file you want to use it in. If you typealias it, you only have to import it in the file you declare the typealias in. So you save yourself so imports... and you keep the unnecessary parts of CoreLocation out of all the rest of your files.

• nhgrif: I fully intended to post more answers. Commenting on the Event class is this important to me, however.
• Isuru: Thanks for the response. Your assumption is indeed correct. The Event class is much more than that. There are lots more properties in the real class. I just stripped it down to the bare minimum to include in the question. I'm getting these events from a web service. The lat and lng values in two separate fields in the JSOn response. So what I basically do is create these Event objects out of that response. The coordinates property is used to easily extract the location as a CLLocationCoordinate2D object.
• nhgrif: Ultimately, your Event class just needs a CLLocationCoordinate2D property (or a typealias for that struct). We should keep the data encapsulated correctly and just write a better JSON parser.
• Isuru: I hear you. I shall get rid of the two separate latitude, longitude properties and create one location property when the object is first created from the JSON. If you have more suggestions like that about other parts of my code, I'd love to hear it. Thank you,
• nhgrif: I'm in the middle of another answer regarding some of the actual meat of the code.