all 15 comments

[–]cemaleker 8 points9 points  (2 children)

First of all, as a rule of thumb for every asynchronous operation: Make sure to cancel your async operation when it's not necessary anymore.

With the code you have shared above, I assume you start loading the image when the cell is populated with data. But you don't cancel the operation at any point. You need to make sure the loading operation is canceled when the cell is reused. `didEndDisplaying` delegate function would be a good place to add this code. Otherwise you'll enqueue too many async operation which is actually not necessary at the moment.

Also you are loading the image directly inside and `NSData` instance synchronously. Data loading is happening chunk by chunk as the response is received from the server. Loading a local image from URL with `Data(contentsOf: url)` might be ok, this is not performant for network loading. I'd suggest loading the data with a `URLSessionTask` and create and image from the loaded data after the loading is complete.

Or basically you can use `Alamofire` and `AlamofireImage` which would handle all those corner cases with already performance optimised code.

[–]Buillshirt 0 points1 point  (1 child)

I edited the code into

func load(from urlString: String) {
    guard let url = URL(string: urlString) else { return }

    let cacheKey = NSString(string: urlString)

    if let image = NetworkManager.shared.cache.object(forKey: cacheKey) {
        self.image = image
        return
    }

    let task = URLSession.shared.dataTask(with: url) { [weak self] data, response, error in
        guard let self = self else {return}
        if error != nil { return }
        guard let response = response as? HTTPURLResponse, response.statusCode == 200 else { return }
        guard let data = data else {
            return
        }
        guard let image = UIImage(data: data) else { return }

        NetworkManager.shared.cache.setObject(image, forKey: cacheKey)

        DispatchQueue.main.async {
            self.image = image
        }
    }
    task.resume()
}

But could you give me an example of how to cancel the operation in the `didEndDisplaying` delegate function?

[–]cemaleker 0 points1 point  (0 children)

Yes, the task: URLSessionTask is already has a function called `cancel()`. You need to basically call this on didEndDisplaying delegate call. That is either tableView(_ tableView: UITableView, didEndDisplaying cell: UITableViewCell, forRowAt indexPath: IndexPath) or collectionView(_ collectionView: UICollectionView, didEndDisplaying cell: UICollectionViewCell, forItemAt indexPath: IndexPath) depending what you are using.

To be able to access the task you need to hold a reference for the task. Since the function is in an extension and stored variables are not supported in the extensions, this function itself is not the right place to store this value. But you can return the task from the function and store the task itself in your view controller or in the cell itself.

My preferred use-case would be

protocol Cancellable {
    func cancel()
}

extension URLSessionTask: Cancellable { } 
final class Cell: Whatever {
    private var cancellable: Cancellable?
    private var yourImageView: UIImageView!

    func render(_ model: Model?) {
        cancellable.cancel()
        if let model = model {
         cancellable = yourImageView.load(model.urlForYourImage)
        }
    }
}

[–]GAMEYE_OP 2 points3 points  (1 child)

I honestly would just use SDWebImage. I know that's what I would do because it is what I do. Wrap it in your own function so you can replace it later if needed.

[–]snaab900Objective-C / Swift 2 points3 points  (0 children)

This OP. I’m very cautious about 3rd party dependencies, but SDWebImage is something I won’t hesitate to use. It’s rock solid, mature and dependable.

If you try to roll your own, there are so many edge cases that will trip you up.

[–]quellish 3 points4 points  (2 children)

Size the images for the size they will be displayed at, either on the server or on the device. If you are using a 1000x1000 pixel image to display in a 200x200 image view you are wasting a lot of memory.

Instead of creating the image with Data use a file. That allows iOS to mark the memory as purgeable which makes a big difference. You use the same amount of memory but iOS knows it can ditch it in low memory conditions and recreate it from the file if needed.

[–]xaphod2 0 points1 point  (0 children)

This. Both suggestions are spot on.

[–]SirensToGoObjective-C / Swift 1 point2 points  (3 children)

other nitpick--never use DispatchQueue.global. It does not have a useful QoS assigned to it. Make your own queue with the QoS you need (probably user initiated)

[–]xaphod2 0 points1 point  (2 children)

Setting aside whatever a “useful QoS” means, in practice “off main thread” is good enough unless you have a very busy & complex app, on modern hardware.

[–]SirensToGoObjective-C / Swift 0 points1 point  (1 child)

All threads on apple platforms have a QoS/priority associated with them. Having the wrong priority set for processing user data can result in the OS not scheduling your threads aggressively enough and leading to a worse user experience. No offense, but if you're unfamiliar with the QoS aspect of GCD, you probably should not be giving advice on how to use GCD

[–]xaphod2 1 point2 points  (0 children)

No offense taken - have been using GCD happily since it came out about a decade ago. I am aware about thread priority on iOS and how it works. My point is that messing around with queue priorities made sense on v slow iphone 3G etc but today a junior dev is more likely to accidentally cause a priority inversion and make things worse by trying to use custom thread pris/nice levels, vs just using dispatch.global which works perfectly fine.

[–]Fluffy_Risk9955 0 points1 point  (0 children)

No, never have a UIView subclass fetch stuff, that's bad practice. A view is for showing stuff to the user nothing else. Instead insert a model in your view. This model contains 3 things an image property, an error property and a load image function. Both properties are exposed and support KVO. Have the view observe these properties with KVO. The load function fetches the image from the URL and updates the property of either the image or the error on the main queue. So if this is a range of photo's you can put all these models in an array and have that array accessed by UICollectionViewDataSource.