2
\$\begingroup\$

I am working with closures and want to confirm that I do not have any memory leaks in my code. Below is my current code:

// in ImageGalleryController.swift
class ImageGalleryCollectionViewController: UICollectionViewController {
  var images = [Image]()

func dropInteraction(_ interaction: UIDropInteraction, performDrop session: UIDropSession) {
    // assignment of closure
    var cellImage = Image() { [weak self](image) in
      self?.images.append(image)
      self?.collectionView.reloadData()
    }
    // asynchronous
    session.loadObjects(ofClass: NSURL.self) { urls in
      if let url = urls.first as? URL {
        cellImage.url = url
      }
    }
    // asynchronous
    session.loadObjects(ofClass: UIImage.self) { images in
      if let image = images.first as? UIImage {
        cellImage.aspectRatio = Double(image.size.height / image.size.width)
      }
    }
  }
}

// in ImageGallery.swift
struct Image {
  var url: URL? = nil {
    didSet {
      if propertiesAreSet() { handler(self) }
    }
  }
  var aspectRatio: Double? = nil {
    didSet {
      if propertiesAreSet() { handler(self) }
    }
  }

  init(handler: @escaping (Image) -> Void) {
    self.handler = handler
  }

  var handler: (Image) -> Void

  private func propertiesAreSet() -> Bool {
    let bool = (url != nil && aspectRatio != nil) ? true : false
    return bool
  }
}

Basically my logic is that when two separate asynchronous requests get and set two separate properties on the Image struct, then I want to append that Image struct to the images array.

Which brings me to my related questions:

  • Since I am fairly new to Swift, am I properly handling memory leaks by declaring [weak self] when I create a new Image instance?
  • Should I set the closure handler to nil after it executes?
  • Is what I am doing ok or am I violating any Swift principles?
\$\endgroup\$
2
\$\begingroup\$

You asked:

... am I properly handling memory leaks by declaring [weak self] when I create a new Image instance?

By using [weak self], yes, you are preventing a strong reference cycle.

By the way, you can always use the “Debug memory graph” feature to confirm whether you have any strong reference cycles. See this answer for information about this feature.

Should I set the closure handler to nil after it executes?

As a general rule, yes,

  1. You should make the handler an optional:

    var handler: ((Image) -> Void)?
    

    and
     

  2. Set it to nil after calling it:

    if propertiesAreSet() { 
        handler?(self) 
        handler = nil
    }
    

The idea is, just in case the caller neglected to use the [weak self] pattern, this will resolve any accidental strong reference cycles. It is a defensive programming approach that minimizes the chances of unresolved strong reference cycles (and also minimizes the chance of introducing a bug that will call the closure multiple times). But this is not used in lieu of [weak self], but rather in conjunction with it.

Is what I am doing ok or am I violating any Swift principles?

Your code works, but is brittle. (This isn’t a “Swift principle” but rather a generally programming one.) For example, these two types are too tightly coupled: What if Image is extended at some future date to have some additional property that can’t be nil? You have to remember to go back to the other types that instantiate Image and make sure you update them accordingly, too, or else you might not see images loaded into your UI at all.

Setting that aside, I’d also suggest that the fact that the handler is called when those two properties are set isn’t very obvious. You really have to carefully read the code of both types to reason about what’s going on.

I’d refactor this, simplifying your model object, the Image, and putting the retrieval logic in the controller.

E.g.

var images = [Image]()

func dropInteraction(_ interaction: UIDropInteraction, performDrop session: UIDropSession) {
    // assignment of closure
    var cellImage = Image()
    let group = DispatchGroup()

    group.enter()
    session.loadObjects(ofClass: NSURL.self) { urls in
        cellImage.url = urls.first as? URL
        group.leave()
    }

    group.enter()
    session.loadObjects(ofClass: UIImage.self) { images in
        if let image = images.first as? UIImage {
            cellImage.aspectRatio = Double(image.size.height / image.size.width)
        }
        group.leave()
    }

    group.notify(queue: .main) { [weak self] in
        guard
            let self = self,
            cellImage.url != nil,
            cellImage.aspectRatio != nil  else { return }

        self.images.append(cellImage)
        self.collectionView.insertItems(at: [IndexPath(item: self.images.count - 1, section: 0)])
    }
}

// in ImageGallery.swift
struct Image {
    var url: URL? = nil
    var aspectRatio: Double? = nil
}

A few other observations:

  1. Notice that above, rather than reloading the whole collection view, you can just reload the new cell.

  2. The above code snippet begs the question as to whether you really should be creating a Image first and retrieving its properties later, or whether you should retrieve the necessary data, and when you have what you need, only then instantiate and return the Image object. It’s hard to get too specific here without more information about this loadObjects method, why it’s taking multiple calls to retrieve the image details, etc. I don’t think we want to get into that level of detail here, but I only mention this as something you should think about as you work on the broader design of your app.

  3. If your reaction to my code snippet above is “gee, I really don’t like adding all of this code to the view controller”, that’s a good intuition. Often, as apps scale, we like to pull this sort of logic out of the view controller and put it in some other object (perhaps a “view model”, perhaps a “presenter”, perhaps “network controller” ... there are lots of ways to skin the cat). But the goal is to limit our view controllers to configuring and responding to views, in the spirit of the “single responsibility principle”. This will make it easier for us to reason about our code and improve its testability (e.g. we want to test our business logic without worrying about collection view behaviors).

    This is a pretty complicated topic and is beyond the scope of this question but the stretch objective is to keep view controllers as minimal as possible. But I might refer you to these sources to get you thinking about these concepts:

    • Dave DeLong is an advocate for not giving up on MVC, but being more prudent in its use. See A Better MVC.
    • Medium has an old survey of some of the MVC alternatives that you’ll see bandied about including MVP, MVVM, VIPER, etc. See iOS Architecture Patterns.
\$\endgroup\$
  • 1
    \$\begingroup\$ Thanks Rob. Your insight is very helpful. \$\endgroup\$ – Darkisa Apr 4 at 15:01

Your Answer

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

Not the answer you're looking for? Browse other questions tagged or ask your own question.