Design pattern: image downloads

Question

This is an image representation of my code below:

This is what they do:

CellWithImageX: Subclasses of UITableViewCell. They have an image and an loading indicator. Only 1 can be shown at the time.

ImageRequester: Downloads the images and gives ImageCache a callback when it's done.

FileExplorer: Saves the images.

ImageCache: Manages everything. Since I am most concerned about the current design, I think the code for the ImageCache is only relevant:

// Used to weakly hold the CellWithImageX image classes. (Is this bad?)
open class WeakObjectWrapper<T: AnyObject> {
    public weak var obj: T?

    public init(obj: T) {
        self.obj = obj
    }

}

class ImageCache {

    // I can not understand how it can work if it isn't a singleton.
    // From every cell, I can easily pass the reference without to much hassle.
    // I am not sure for an singleton alternative.
    static let sharedInstance = ImageCache()

    // TODO: Maybe remove some elements from time to time?
    // Yes, this is an array of the images in the table view cell...
    private var loadableImages: [WeakObjectWrapper<LoadableImage>] = []

    private init() {}

    func handleImage(photoIdentifier: Int64, loadableImage: LoadableImage) {
        // Bit concenered about a race condition.
        // I read about DispatchQueues and Semaphores, can't really
        // decide what would be the best way.
        assert(Thread.isMainThread)
        // LoadableImage has an identifier which we can use to later on match the downloaded image.
        loadableImage.identifier = photoIdentifier

        guard photoIdentifier != 0 else {
            loadableImage.show(image: #imageLiteral(resourceName: "anonymous"))

            return
        }

        guard let image = determineImage(photoIdentifier: photoIdentifier) else {
            loadableImages.append(WeakObjectWrapper(obj: loadableImage))
            loadableImage.showIndicator()

            return
        }

        loadableImage.show(image: image)
    }

    func received(image: UIImage, forPhotoIdentifier: Int64) {
        assert(Thread.isMainThread)

        FileExplorer.sharedInstance.save(image: image.pngData()!, photoIdentifier: forPhotoIdentifier) // This is validated that this works, we can safely force unwrap.

        for loadableImage in loadableImages.filter({ $0.obj?.identifier == forPhotoIdentifier }) {
            // While iterating it may occur that the reference is gone.
            loadableImage.obj?.show(image: image)
        }
    }

    private func determineImage(photoIdentifier: Int64) -> UIImage? {
        guard let image = FileExplorer.sharedInstance.determineImage(photoIdentifier: photoIdentifier) else {
            ImageRequester.sharedInstance.download(photoIdentifier: photoIdentifier)

            return nil
        }

        return image
    }
}

In my cellForRowAt method in my tableView, I am calling the ImageCache handleImage method on my cell.loadableImage property.

My concerns:

• I am using a few singletons: ImageRequester, ImageCache and FileExplorer. I am not sure if this is the best approach, but I can not imagine a better way because this way, all my cells can easily register themselfs.
• I have a array of weak objects which references to the loadable images inside of a tableViewCell. Isn't there a better way?
• I am asserting I am on the main thread. If I am on multiple threads, race conditions may occur while I loop through the array. I looked into DispatchQueues and Semaphores. I just didn't found the right way to do it, and what the best way would be in my case. Does someone has a suggestion about what way would be a good way in my case?

Answer 1

The way I see it your requirements can be summarized as:

1. Have a central repository of images.
2. Load Images from a some data source.
3. Save Persist Images.
4. Give a cell its appropriate image based on an identifier.

The main issue I see is that the cells are tightly coupled to the ImageCache object, it might be better to have some sort of ViewModel or controller layer sit between your image cache and cells. This model or controller would contain the information needed to retrieve an image for image cache along with the information needed to display the image to the cell. This would make your code much more testable and decouple your UITableViewCells from the the your singleton cache.

On whether a singleton is necessary-- I am going to take a guess that this is all for one ViewController. Why not just initialize the ImageCache object in that ViewController? If this is truly used across many ViewControllers than I could maybe be convinced to get behind the use of a singleton.

On the WeakReference array... that is valid and actually relatively advanced pattern in Swift, I'm not sure that you really need it here though although its a little hard to tell without more context. It appears LoadableImage is some sort of object that encapsulates a callback that displays the image or an indicator on the UI... in which case yes you would need a weak reference, although I am unsure as to why you even need to store the LoadableImages in an array to begin with. I think that you could abstract away the array if you refactored to a more traditional Callback oriented approach.

In a callback approach the TableViewCells would merely request the appropriate image and show the loading indicator until the callback was called. If you really needed to cache the images, you could do so in an array within the cache.

I wish that your method names were more descriptive in Swift style... for example determineImage(for photoId:String). That's part of the expressiveness and readability of the language. Another example the function received doesn't really express what it's use is I think that its some sort of callback that will display an image once it has been loaded and display that image to a specific LoadableImage, but this needs to be more clear.

I think to give you more advice you need to post more code, along with a description of what the requirements are. For example: How many ViewControllers are using the Cache? Do you need to store images across the lifecycle of multiple ViewControllers? How are you planning on linking the ImageIdentifiers to the TableViewCells.

If you post a few more details, I will follow up with a few code samples, but as it is I don't see how I can give you any tangible direction without knowing a little bit more.