3
\$\begingroup\$

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 <String, UIView>()
        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.constraintsWithVisualFormat("H:|-0-[control]-0-|", options: NSLayoutFormatOptions(0), metrics: nil, views: viewsDict))
        self.view.addConstraints(NSLayoutConstraint.constraintsWithVisualFormat("V:|-0-[scrollView(400)]-[control]-0-|", options: NSLayoutFormatOptions(0), metrics: nil, views: viewsDict))

        // Set up the array to hold the views for each page
        for (var i = 0; i < pageCount; ++i) {
            self.pageViews.append(nil)
            self.pageImages.append(nil)
            self.imagesDownloads.append(nil)
            self.downloadsResponse.append(nil)
        }

    }

    required init(coder aDecoder: NSCoder) {
        fatalError("init(coder:) has not been implemented")
    }

    func loadVisiblePages() {
        // First, determine which page is currently visible
        var pageWidth:CGFloat = self.scrollView.frame.size.width;
        var page = Int(floor((self.scrollView.contentOffset.x * 2.0 + pageWidth) / (pageWidth * 2.0)));

        /* 
            Check that page have changed, in case that user drag left in first page, or drag right in last page
            a 'scrollViewDidEndDecelerating' is fired
        */
        if self.viewingPage != page {

            self.viewingPage = page
            // Update the page control
            self.pageControl.currentPage = page;

            // Work out which pages we want to load
            var firstPage = page - 1;
            var lastPage = page + 1;

            // Purge anything before the first page
            for (var i=0; i<firstPage; i++) { self.purgePage(i) }
            for (var i=firstPage; i<=lastPage; i++) { self.loadPage(i) }
            for (var i = lastPage+1 ; i < self.pageImages.count ; i++) { self.purgePage(i) }
        }

    }

    func loadPage(page:Int) {
        if page < 0 || page >= self.pageImages.count {
            // If it's outside the range of what we have to display, then do nothing
            return;
        }

        // Load an individual page, first seeing if we've already loaded it
        var pageView:UIView? = self.pageViews[page];
        if pageView == nil {
            var frame:CGRect = self.scrollView.bounds;
            frame.origin.x = frame.size.width * CGFloat(page)
            frame.origin.y = 0.0

            var newPageView:ImageDownloadView = ImageDownloadView()
            newPageView.frame = frame;

            if(self.imagesDownloads[page] == nil && self.pageImages[page] == nil) {
                self.downloadsResponse[page] = { (pageNumber:Int!, actionType:ImageDownload.ActionType, progress:Float, result:UIImage!) -> Void in
                    switch actionType {
                    case ImageDownload.ActionType.STARTING :
                        if self.pageViews[pageNumber] != nil {
                            //DO NOTHING FOR NOW
                        }
                    case ImageDownload.ActionType.DOWNLOADING :
                        if self.pageViews[pageNumber] != nil {
                            self.pageViews[pageNumber]?.setDownloadProgress(progress)
                        }
                    case ImageDownload.ActionType.COMPLETED :
                        self.pageImages[pageNumber] = result;
                        if self.pageViews[pageNumber] != nil {
                            self.pageViews[pageNumber]?.showImage(result)
                        }
                    }
                }
                self.imagesDownloads[page] = ImageDownload()
                self.imagesDownloads[page]?.connect(self.imagesUrls![page], pageNumber:page, downloadReponse:self.downloadsResponse[page]!)
            } else if self.pageImages[page] != nil {
                newPageView.showImage(self.pageImages[page]!)
            }

            self.scrollView.addSubview(newPageView)
            self.pageViews[page] = newPageView
        }
    }

    func purgePage(page:Int) {
        if page < 0 || page >= self.pageImages.count {
            // If it's outside the range of what we have to display, then do nothing
            return;
        }

        // Remove a page from the scroll view and reset the container array
        var pageView:UIView? = self.pageViews[page];
        if pageView != nil {
            pageView?.removeFromSuperview()
            self.pageViews[page] = nil
        }
    }

    override func viewDidAppear(animated:Bool) {
        super.viewDidAppear(animated)

        // Set up the content size of the scroll view
        var pagesScrollViewSize:CGSize = self.scrollView.frame.size;
        self.scrollView.contentSize = CGSizeMake(pagesScrollViewSize.width * CGFloat(self.pageImages.count), pagesScrollViewSize.height);

        // Load the initial set of pages that are on screen
        self.loadVisiblePages()
    }

    override func didReceiveMemoryWarning() {
        super.didReceiveMemoryWarning()
        //Clean not visible images ?
    }

    func scrollViewDidEndDecelerating(scrollView: UIScrollView!) {
        self.loadVisiblePages()
    }
}

ImageDownloadView

class ImageDownloadView: UIView {
    var imageViewer:UIImageView = UIImageView()
    var roundProgress:RoundProgress = RoundProgress(frame:CGRectMake(0, 0, 60, 60))

    override init() {
        super.init()

        self.imageViewer.contentMode = UIViewContentMode.ScaleAspectFit;

        self.imageViewer.setTranslatesAutoresizingMaskIntoConstraints(false)
        self.roundProgress.setTranslatesAutoresizingMaskIntoConstraints(false)
        self.addSubview(self.imageViewer)
        self.addSubview(self.roundProgress)

        //Set layout
        var viewsDict = Dictionary <String, UIView>()
        viewsDict["image"] = self.imageViewer
        viewsDict["progress"] = self.roundProgress

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

        self.addConstraints(NSLayoutConstraint.constraintsWithVisualFormat("H:[progress(60)]", options: NSLayoutFormatOptions(0), metrics: nil, views: viewsDict))
        self.addConstraints(NSLayoutConstraint.constraintsWithVisualFormat("V:[progress(60)]", options: NSLayoutFormatOptions(0), metrics: nil, views: viewsDict))


        self.addConstraint(NSLayoutConstraint(item:self.roundProgress,
            attribute:NSLayoutAttribute.CenterX,
            relatedBy:NSLayoutRelation.Equal,
            toItem:self,
            attribute:NSLayoutAttribute.CenterX,
            multiplier:1,
            constant:0))

        self.addConstraint(NSLayoutConstraint(item:self.roundProgress,
            attribute:NSLayoutAttribute.CenterY,
            relatedBy:NSLayoutRelation.Equal,
            toItem:self,
            attribute:NSLayoutAttribute.CenterY,
            multiplier:1,
            constant:0))
    }

    func setDownloadProgress(progress:Float) {
        self.roundProgress.setProgress(progress / 100)
    }

    func showImage(image:UIImage) {
        self.imageViewer.image = image
        self.roundProgress.removeFromSuperview()
    }

    required init(coder aDecoder: NSCoder) {
        fatalError("init(coder:) has not been implemented")
    }

    override init(frame: CGRect) {
        super.init(frame: frame)
    }

}

ImageDownload

typealias DownloadResponse = (pageNumber:Int!, actionType:ImageDownload.ActionType, progress:Float, result:UIImage!) -> Void

class ImageDownload:NSObject, NSURLConnectionDataDelegate {

    var data = NSMutableData()
    var pageNumber:Int?
    var downloadReponse:DownloadResponse?
    var contentLength:Int64?

    enum ActionType:CInt {
        case STARTING = 1, DOWNLOADING, COMPLETED
    }

    func connect(query:String, pageNumber:Int, downloadReponse:DownloadResponse) {
        self.pageNumber = pageNumber
        self.downloadReponse = downloadReponse

        var url =  NSURL.URLWithString(query)
        var request = NSURLRequest(URL: url)
        var conn = NSURLConnection(request: request, delegate: self, startImmediately: false)
        conn.start()
    }

    func connection(didReceiveResponse: NSURLConnection!, didReceiveResponse response: NSURLResponse!) {
        contentLength = response.expectedContentLength
        downloadReponse!(pageNumber: self.pageNumber, actionType: ActionType.STARTING, progress:0.0, result: nil)
    }

    func connection(connection: NSURLConnection!, didReceiveData conData: NSData!) {
        self.data.appendData(conData)
        var progress:Float = Float(self.data.length * 100) / Float(self.contentLength!)
        downloadReponse!(pageNumber: self.pageNumber, actionType: ActionType.DOWNLOADING, progress:progress, result: nil)
    }

    func connectionDidFinishLoading(connection: NSURLConnection!) {
        downloadReponse!(pageNumber: self.pageNumber, actionType: ActionType.COMPLETED, progress:100.0, result: UIImage(data: self.data))
    }

}

RoundProgress

class RoundProgress:UIView {
    var progressCircle = CAShapeLayer();
    var progress:Float = 0.1;

    override init(frame: CGRect) {
        super.init(frame: frame)
        self.bounds = frame
    }


    override func didMoveToSuperview() {
        super.didMoveToSuperview()

        let centerPoint = CGPoint (x: self.bounds.width / 2, y: self.bounds.width / 2);
        let circleRadius : CGFloat = self.bounds.width / 2 * 0.83;
        var circlePath = UIBezierPath(arcCenter: centerPoint, radius: circleRadius, startAngle: CGFloat(-0.5 * M_PI), endAngle: CGFloat(1.5 * M_PI), clockwise: true    );

        progressCircle.path = circlePath.CGPath;
        progressCircle.strokeColor = UIColor.whiteColor().CGColor;
        progressCircle.fillColor = UIColor.clearColor().CGColor;

        progressCircle.lineWidth = 2.5;
        progressCircle.strokeStart = 0;
        progressCircle.strokeEnd = 0.0;

        self.layer.addSublayer(progressCircle);

    }

    func setProgress(progress:Float) {
        self.progress = progress
        progressCircle.strokeEnd = CGFloat(self.progress)
        self.setNeedsDisplay()
    }


    required init(coder aDecoder: NSCoder) {
        fatalError("init(coder:) has not been implemented")
    }


}
\$\endgroup\$
  • \$\begingroup\$ Welcome! We cannot help with the last request as it appears to be unimplemented code (at least based on the request), but we can help with everything else (if everything works). \$\endgroup\$ – Jamal Sep 6 '14 at 18:05
  • \$\begingroup\$ the code is working like charm, a code review will be nice :), for 'didReceiveMemoryWarning' a suggestion will work nice :) \$\endgroup\$ – shay te Sep 6 '14 at 18:08
  • \$\begingroup\$ This is a ton of code. We can review it, but with this much code, you're likely only going to get some reviews of the big picture. If you want very specific sections reviewed, I'd suggest posting a different question and include just those specific questions. \$\endgroup\$ – nhgrif Sep 6 '14 at 18:21
  • \$\begingroup\$ @nhgrif thanks for the edit, i am most worry about the 'ImageDownload' class, and the callback, thanks again \$\endgroup\$ – shay te Sep 6 '14 at 18:32
4
\$\begingroup\$

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.

| improve this answer | |
\$\endgroup\$

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.