Ggthjy

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.

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.