HiveBrain v1.2.0
Get Started
← Back to all entries
patternswiftMinor

Images viewer from URLs with download progress

Submitted by: @import:stackexchange-codereview··
0
Viewed 0 times
withviewerprogressdownloadimagesfromurls

Problem

I've created an imageViewer that will be initialized with a String array, and will download and show the images as needed.

Alongside a review, could you give me a suggestion on what to do in didReceiveMemoryWarning?

PagedScrollViewController

```
class PagedScrollViewController:UIViewController,UIScrollViewDelegate {

var imagesUrls:[String]?
var imagesDownloads:[ImageDownload?] = [ImageDownload]()
var downloadsResponse:[DownloadResponse?] = [DownloadResponse?]()

var pageImages:[UIImage?] = [UIImage]()
var pageViews:[ImageDownloadView?] = [ImageDownloadView]()

var scrollView:UIScrollView = UIScrollView()
var pageControl:UIPageControl = UIPageControl()

var viewingPage = -1

init(images:[String]) {
super.init(nibName: nil, bundle: nil)

self.imagesUrls = images
self.title = "Image viewer";

self.view.backgroundColor = UIColor.blackColor()

var pageCount = self.imagesUrls!.count

self.scrollView.pagingEnabled = true
self.scrollView.delegate = self
self.scrollView.showsHorizontalScrollIndicator = false
self.scrollView.showsVerticalScrollIndicator = false

// Set up the page control
self.pageControl.currentPage = 0;
self.pageControl.numberOfPages = pageCount;

//Add
self.pageControl.setTranslatesAutoresizingMaskIntoConstraints(false)
self.scrollView.setTranslatesAutoresizingMaskIntoConstraints(false)

self.view.addSubview(self.pageControl)
self.view.addSubview(self.scrollView)

//Set layout
var viewsDict = Dictionary ()
viewsDict["control"] = self.pageControl;
viewsDict["scrollView"] = self.scrollView;

self.view.addConstraints(NSLayoutConstraint.constraintsWithVisualFormat("H:|-0-[scrollView]-0-|", options: NSLayoutFormatOptions(0), metrics: nil, views: viewsDict))
self.view.addConstraints(NSLayoutConstraint.constraintsWithVisua

Solution

Your ImageDownload class has some issues.

As a start, I'll point out that I don't like the name. This should be more like ImageDownloader, but this is a rather minor complaint.

The bigger problem here is that we take one completion block and that completion block is expected to respond to 3 different steps along the way to downloading an image. And perhaps the larger problem is that we don't even care in the slightest that the download might fail. We've not included the delegate methods for the connection failure condition.

We also don't expose any means for allowing the user of the object to cancel the download request (for example, when the view scrolls of screen). As such, we're potentially wasting our end user's mobile data if they're not on wi-fi.

So we need to add a means for canceling the download. We also need a means of handling the asynchronous case of the download failing or being canceled.

Resist the urge to add two more states to your enum and drastically increase the complexity of your completion block. Instead, we need to implement a protocol-delegate pattern.

protocol ImageDownloaderDelegate {
    required func imageDownloaderDidConnect()
    required func imageDownloaderDidComplete(image: UIImage)
    optional func imageDownloaderDidReceiveData(progress: Float)
    optional func imageDownloaderDidFail(error: NSError)
    optional func imageDownloaderWasCanceled()
}


And obviously add any additional arguments to these methods as you feel appropriate.

Now we add a value to our ImageDownloader class:

var delegate: ImageDownloaderDelegate?


And we call these method on the delegate when appropriate. We should be sure to use optional chaining when calling these method, particularly on the optional methods:

self.delegate?.imageDownloaderDidReceiveData?(0.3)


Of course, this means that whoever is using the ImageDownloader now needs to implement these methods and be prepared to respond to them when they're called.

This saves us from the big ugly switch, which is made uglier by it being in a closure, and would only get uglier when you add cases for the other two scenarios I point out are missing. Instead, we just have these 2-5 methods implemented. Overall, it's not any fewer lines of code. It's about the same. But each method is out on it's own. They're smaller and more compartmentalized. These are 5 very distinct events, and they needed to be treated as such.

There's a very good reason why NSURLConnection uses the protocol-delegate pattern, and we shouldn't try to squash it all into a single completion block.

Code Snippets

protocol ImageDownloaderDelegate {
    required func imageDownloaderDidConnect()
    required func imageDownloaderDidComplete(image: UIImage)
    optional func imageDownloaderDidReceiveData(progress: Float)
    optional func imageDownloaderDidFail(error: NSError)
    optional func imageDownloaderWasCanceled()
}
var delegate: ImageDownloaderDelegate?
self.delegate?.imageDownloaderDidReceiveData?(0.3)

Context

StackExchange Code Review Q#62144, answer score: 4

Revisions (0)

No revisions yet.