Tracking a geolocation

I have created this ViewController using Swift. Its job is to track geolocation and draw a line on a map and save it to core data object. I just want some advice on improving the geolocation tracking using core location and storing information using core data.

```
import CoreLocation
import MapKit
import UIKit
import CoreData

class ViewController: UIViewController,CLLocationManagerDelegate,MKMapViewDelegate {

@IBOutlet weak var mapView: MKMapView!
@IBOutlet weak var lblCurrentLocation: UILabel!

var locationManager = CLLocationManager()
var userLocations:[CLLocation] = []

override func viewDidLoad() {
super.viewDidLoad()
// Do any additional setup after loading the view, typically from a nib.

//setup coreLocation manager
self.locationManager.requestAlwaysAuthorization()
self.locationManager.requestWhenInUseAuthorization()
locationManager.delegate = self
locationManager.desiredAccuracy = kCLLocationAccuracyNearestTenMeters

//setup map
mapView.delegate = self;
mapView.mapType = MKMapType.Standard
mapView.showsUserLocation = true
}

func locationManager(manager: CLLocationManager, didUpdateLocations locations: [CLLocation]) {
lblCurrentLocation.text = "\(locations[0])"
userLocations.append(locations[0] as CLLocation)

let spanX = 0.007
let spanY = 0.007

let newRegin = MKCoordinateRegion(center: mapView.userLocation.coordinate, span:MKCoordinateSpanMake(spanX, spanY))
mapView.setRegion(newRegin, animated: true)

if (userLocations.count > 1){
let sourceIndex = userLocations.count - 1
let destinationIndex = userLocations.count - 2

let c1 = userLocations[sourceIndex].coordinate
let c2 = userLocations[destinationIndex].coordina

You already know exactly what your viewDidLoad method should look like... you just didn't write it how you know it should look. It should look like this:

override func viewDidLoad() {
    super.viewDidLoad()

    setUpCoreLocationManager()
    setUpMap()
}

If you've got a block of code and it's got a comment over top telling you what it does, then you've got a block of code and a method name... you just need to refactor it to be that way.

var locationManager =  CLLocationManager()

This should probably be a let. I can't see any reason why we'd want to change this.

userLocations.append(locations[0] as CLLocation)

This cast is unnecessary, and I'm kind of surprised that Xcode doesn't give a warning for this (maybe it does). But I'm also concerned about locations[0]. The documentation may guarantee that the locations array always has at least one object in it, but to know that, I must look at Apple's documentation. Moreover, the rules could change, and now all of a sudden, you'd have a crash.

I'd prefer seeing this:

if location = locations.first {
    userLocations.append(location)
}

In general, this method func locationManager(manager: CLLocationManager, didUpdateLocations locations: [CLLocation]) is doing way too much. I'd prefer to see it look something more like this:

func locationManager(manager: CLLocationManager, didUpdateLocations locations: [CLLocation]) {
    if let location = locations.first {
        userLocations.append(location)
        updateMap()
    }
}

Where updateMap() is a method that draws the line you're drawing in the rest of this method's body.

In this method, func mapView(mapView: MKMapView, rendererForOverlay overlay: MKOverlay) -> MKOverlayRenderer, I might reverse the nesting with a guard:

guard overlay is MKPolyline else {
   return MKPolylineRenderer() 
}

let polylineRenderer = MKPolylineRenderer(overlay: overlay)
polylineRenderer.strokeColor = UIColor.blueColor()
polylineRenderer.lineWidth = 4
return polylineRenderer

Although, I'd also make the case that we could write a convenience initializer for MKPolylineRenderer, so those last four lines just get turned into:

return MKPolylineRenderer(overlay: overlay, color: UIColor.blueColor(), lineWidth: 4)

override func didReceiveMemoryWarning() {
    super.didReceiveMemoryWarning()
    // Dispose of any resources that can be recreated.
}

If you're overriding a method just to call super, you don't need the method implemented in this class at all. Just omit this method from the source code altogether.

//setup coreLocation manager 
self.locationManager.requestAlwaysAuthorization()
self.locationManager.requestWhenInUseAuthorization()
locationManager.delegate = self
locationManager.desiredAccuracy = kCLLocationAccuracyNearestTenMeters

You should be consistent with your usage of self, and the general feeling of the Swift community is that you should omit it when it's unnecessary.

Also, asking for both Always and When In Use authorizations here is pointless. You can only ask for one or the other. And, you can only ask once.

I highly recommend that you take a completely different approach here though. First, you should explain to the user that you're about to ask for permission and explain why. And here, give them the option to opt out completely. Once you ask the OS for permission, you can never ask again, so if the user denies, you can't get the OS to prompt them ever again.

You should have a pop up that has text something like the following:


We need to gather your location data in order to . How do you want to share your location data?

• Always

• Only when app is open

• Never

If they choose always, then you request always authorization.

If they choose only when app is open, then you request when in use authorization.

If they choose never, you do neither, and maybe you need to dismiss this view controller... but then, you don't ask the OS for anything. You wait until the next time they come to this screen, and you just ask them again.

This approach to asking users for permissions leads in a much higher likelihood of the user actually giving you permission when you make the request through the OS, and allows you the opportunity to ask again if they didn't allow it the first time through.

Finally, you have some issues with optionals, style, and indentation in this file. You can install and run swiftlint to take care of most of this automatically and generate warnings in Xcode for the rest so you can easily see where you have problems to correct.

StackExchange Code Review Q#124110, answer score: 4