3
\$\begingroup\$

I registered a CustomMapAnnotationView and a ClusterView: the first one simply extends MKMarkerAnnotationView and overrides annotation this way:

override var annotation: MKAnnotation? {
        willSet {
        // CustomMapAnnotation is based on MKPointAnnotation
            if let c = value as? CustomMapAnnotation {
                clusteringIdentifier = "c"
                markerTintColor = UIColor.orange
                displayPriority = .defaultLow
            }
        }
    }

ClusterView is a MKAnnotationView that is used when annotations can be grouped.

 mapView.register(CustomMapAnnotationView.self, forAnnotationViewWithReuseIdentifier: MKMapViewDefaultAnnotationViewReuseIdentifier)
 mapView.register(ClusterView.self, forAnnotationViewWithReuseIdentifier: MKMapViewDefaultClusterAnnotationViewReuseIdentifier)

Now the code review part.

I use CustomMapAnnotationView and ClusterView this way:

func mapView(_ mapView: MKMapView, viewFor annotation: MKAnnotation) -> MKAnnotationView? {

        guard !(annotation is MKUserLocation) else {
            return nil
        }

        if(annotation is CustomMapAnnotation){

            let annotationID = "c"

            var annotationView: CustomMapAnnotationView?

            if let dequeuedAnnotationView = mapView.dequeueReusableAnnotationView(withIdentifier: annotationID) as? CustomMapAnnotationView {
                annotationView = dequeuedAnnotationView
                annotationView!.annotation = annotation
            }
            else
            {
                annotationView = CustomMapAnnotationView(annotation: annotation, reuseIdentifier: annotationID)
                annotationView?.rightCalloutAccessoryView = UIButton(type: .detailDisclosure)
            }

            if let annotationView = annotationView {
                annotationView.canShowCallout = true
            }

            return annotationView

        }else{

            let clusterID = "Cluster"
            var clusterView = mapView.dequeueReusableAnnotationView(withIdentifier: clusterID)
            if clusterView == nil {
                clusterView = ClusterView(annotation: annotation, reuseIdentifier: clusterID)
            } else {
                clusterView?.annotation = annotation
            }

            return clusterView
        }
        }

This code perfectly works or at least it does what I wanted it to do. But I'm not sure this is correct way to perform this action inside the body of this function. Any suggestion is appreciated.

\$\endgroup\$
5
\$\begingroup\$

A couple of observations:

  1. You have code that says:

    var annotationView: CustomMapAnnotationView?
    
    if let dequeuedAnnotationView = mapView.dequeueReusableAnnotationView(withIdentifier: annotationID) as? CustomMapAnnotationView {
        annotationView = dequeuedAnnotationView
        annotationView!.annotation = annotation
    }
    else
    {
        annotationView = CustomMapAnnotationView(annotation: annotation, reuseIdentifier: annotationID)
        ...
    }
    

    In iOS 11 and later, the mapView.dequeueReusableAnnotationView(withIdentifier:for:) can reduce that to a single line:

    let annotationView = mapView.dequeueReusableAnnotationView(withIdentifier: annotationID, for: annotation)
    
  2. Your mapView(_:viewFor:) is configuring the annotation views. I’d move that configuration code into the MKAnnotationView implementations for better separation of responsibilities and eliminating cruft from your view controller. For example:

    class CustomMapAnnotationView: MKMarkerAnnotationView {
        static let clusteringIdentifier = Bundle.main.bundleIdentifier! + ".c"
    
        override init(annotation: MKAnnotation?, reuseIdentifier: String?) {
            super.init(annotation: annotation, reuseIdentifier: reuseIdentifier)
    
            clusteringIdentifier = CustomMapAnnotationView.clusteringIdentifier
            markerTintColor = .orange
            displayPriority = .defaultLow
            collisionMode = .circle
            rightCalloutAccessoryView = UIButton(type: .detailDisclosure)
        }
    
        required init?(coder aDecoder: NSCoder) {
            fatalError("init(coder:) has not been implemented")
        }
    
        override var annotation: MKAnnotation? {
            willSet {
                clusteringIdentifier = CustomMapAnnotationView.clusteringIdentifier
            }
        }
    }
    

    And

    class ClusterView: MKMarkerAnnotationView {
        override init(annotation: MKAnnotation?, reuseIdentifier: String?) {
            super.init(annotation: annotation, reuseIdentifier: reuseIdentifier)
            displayPriority = .required
            collisionMode = .circle
            markerTintColor = .purple
        }
    
        required init?(coder aDecoder: NSCoder) {
            fatalError("init(coder:) has not been implemented")
        }
    }
    

    This cleans up your mapView(_:viewFor:) and keeps the annotation view configuration code where it belongs.

  3. Consider your registration of these classes:

    mapView.register(CustomMapAnnotationView.self, forAnnotationViewWithReuseIdentifier: MKMapViewDefaultAnnotationViewReuseIdentifier)
    mapView.register(ClusterView.self, forAnnotationViewWithReuseIdentifier: MKMapViewDefaultClusterAnnotationViewReuseIdentifier)
    

    Note, you’re registering these identifiers, so your mapView(_:viewFor:) shouldn’t then use its own reuse identifiers of c and Cluster, respectively.

  4. But the whole point of MKMapViewDefaultAnnotationViewReuseIdentifier, etc., in iOS 11 and later is so that you don’t have to implement mapView(_:viewFor:) at all. So you can comment out that whole routine (now that you’ve moved the annotation view configuration code to the annotation views, themselves, where it belongs).

    That having been said, sometimes you need a mapView(_:viewFor:). For example, maybe you need to support iOS versions prior to 11. Or perhaps you have more than three types of annotation types (your main custom annotation, your cluster annotation, and MKUserLocation). But then you wouldn’t register MKMapViewDefaultAnnotationViewReuseIdentifier as the reuse identifier. You’d register your own reuse identifier.

    If you do use your own reused identifier, I would have a static property in my annotation view with the preferred reuse identifier, rather than sprinkling hardcoded strings throughout my codebase. E.g.

    class CustomMapAnnotationView: MKMarkerAnnotationView {
        static let preferredReuseIdentifier = Bundle.main.bundleIdentifier! + ".customMapAnnotationView”
    
        ...
    }
    

    And

    mapView.register(CustomMapAnnotationView.self, forAnnotationViewWithReuseIdentifier: CustomMapAnnotationView.preferredReuseIdentifier)
    

    And

    extension ViewController: MKMapViewDelegate {
        func mapView(_ mapView: MKMapView, viewFor annotation: MKAnnotation) -> MKAnnotationView? {
            switch annotation {
            case let annotation as CustomMapAnnotation:
                return mapView.dequeueReusableAnnotationView(withIdentifier: CustomMapAnnotationView.preferredReuseIdentifier, for: annotation)
    
            ...
            }
        }
    }
    

    But that’s not needed in your case. You can remove mapView(_:viewFor:) entirely.

\$\endgroup\$

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service, privacy policy and cookie policy