6
\$\begingroup\$

I built an app that allows users to add and remove images in a collection view. I want this to be useful online so I save the images into a database and retrieve them every time. I'm looking for constructive criticism on how I solved this problem. It is in swift. The code works just fine, but I notice some lag sometimes, and it may be because of the way I designed the software. This is why I'm looking for criticism.

ViewGallery.swift

import UIKit
import Firebase
import DKImagePickerController

class ViewGallery: UIViewController, UICollectionViewDataSource, UICollectionViewDelegate, UICollectionViewDelegateFlowLayout {

let db = Firebase(url: "mydatabaseurl")
@IBOutlet var collectionView: UICollectionView!

 var imageDatas: [String] = []
 var selectedItems: [NSIndexPath] = []
 var keys: [AnyObject] = []

 override func viewDidLoad() {
  super.viewDidLoad()
  db.childByAppendingPath("Images").observeEventType(.Value, withBlock: {
    snapshot in
    let images = snapshot.value as! NSDictionary
    self.keys = images.allKeys
    self.imageDatas = []
    for key in self.keys {
      self.imageDatas.append(images[key as! NSString as String]!["image"] as! String)
    }
    self.selectedItems = []
    self.collectionView.reloadData()
  })
}

@IBAction func addPictures(sender: AnyObject) {
  let pickerController = DKImagePickerController()

  pickerController.didSelectAssets = { [unowned self] (assets: [DKAsset]) in
    print("didSelectAssets")
    for asset in assets {
      let imageData = asset.rawData?.base64EncodedStringWithOptions(.Encoding64CharacterLineLength)
      self.saveImageStringToDatabase(imageData!)
    }
  }

  self.presentViewController(pickerController, animated: true, completion: nil)
}

@IBAction func removePicture(sender: AnyObject) {
  for indexPath in selectedItems {
    db.childByAppendingPath("Images").childByAppendingPath(self.keys[indexPath.item] as! String).removeValue()
  }
}

func saveImageStringToDatabase(imageString: String) {
  let value = ["image": imageString]
  db.childByAppendingPath("Images").childByAutoId().setValue(value)
}

func collectionView(collectionView: UICollectionView, numberOfItemsInSection section: Int) -> Int {
  if imageDatas.count == 0 {
    return 0
  } else {
    return imageDatas.count
  }
}

func collectionView(collectionView: UICollectionView, cellForItemAtIndexPath indexPath: NSIndexPath) -> UICollectionViewCell {
  if imageDatas.count == 0 {
    let cell = collectionView.dequeueReusableCellWithReuseIdentifier("View Gallery Cell", forIndexPath: indexPath)
    return cell
  } else {
    let base64String = self.imageDatas[indexPath.item]
    let imageData = NSData(base64EncodedString: base64String, options: .IgnoreUnknownCharacters)
    let decodedImage = UIImage(data: imageData!)
    let cell = collectionView.dequeueReusableCellWithReuseIdentifier("View Gallery Cell", forIndexPath: indexPath) as! ImageCell
    cell.imageView.image = decodedImage
    return cell
  }
}

func collectionView(collectionView: UICollectionView, didSelectItemAtIndexPath indexPath: NSIndexPath) {
  let cell = self.collectionView.cellForItemAtIndexPath(indexPath) as! ImageCell
  if selectedItems.contains(indexPath) {
    print("unselected")
    cell.backgroundColor = UIColor.clearColor()
    let indexToBeDeleted = selectedItems.indexOf(indexPath)
    selectedItems.removeAtIndex(indexToBeDeleted!)
  } else {
    print("selected")
    cell.backgroundColor = UIColor.redColor()
    selectedItems.append(indexPath)
  }
}

}
\$\endgroup\$
  • \$\begingroup\$ Purely "FTR". It's probably better to use Parse than Firebase for just this situation. \$\endgroup\$ – Fattie Dec 15 '15 at 0:00
4
\$\begingroup\$

I am not familiar with the Firebase or DKImagePickerController framework, so I cannot say anything about that. But there are some things that I noticed:

There is no need in Swift to reference properties via self (unless there is an ambiguity with a local variable, e.g. in an init method). And you are not consistent here, e.g. both self.selectedItems and selectedItems is used.


This

func collectionView(collectionView: UICollectionView, numberOfItemsInSection section: Int) -> Int {
    if imageDatas.count == 0 {
        return 0
    } else {
        return imageDatas.count
    }
}

can be simplified to

func collectionView(collectionView: UICollectionView, numberOfItemsInSection section: Int) -> Int {
    return imageDatas.count
}

And in

func collectionView(collectionView: UICollectionView, cellForItemAtIndexPath indexPath: NSIndexPath) -> UICollectionViewCell

there is no need to check for imageDatas.count == 0 (and return a dummy cell in that case). If the array is empty then numberOfItemsInSection returns zero, i.e. the collection view displays no item at all and cellForItemAtIndexPath will not be called.


If the order of the selected items is not relevant then you can use a set instead of an array:

var selectedItems = Set<NSIndexPath>()

Adding or removing an index path then simplifies slightly to

if selectedItems.contains(indexPath) {
    cell.backgroundColor = UIColor.clearColor()
    selectedItems.remove(indexPath)
} else {
    cell.backgroundColor = UIColor.redColor()
    selectedItems.insert(indexPath)
}

A loop like

self.imageDatas = []
for key in self.keys {
    self.imageDatas.append(...)
}

can be replaced by a mapping:

self.imageDatas = self.keys.map { key in ... }
\$\endgroup\$
  • \$\begingroup\$ Awesome, thank you for taking the time to look at my code. Is it best practice to turn images into base64 string and saving it into a database, then to view it by retrieving the base64 string and converting it back into an image? What do you think? Is there a better way to do it? \$\endgroup\$ – user3904534 Dec 14 '15 at 22:28
  • \$\begingroup\$ @user3904534: Usually storing the image data directly as a binary blob is to be preferred. That saves space and computation. But as I said, I don't know Firebase, therefore I did not cover that part. \$\endgroup\$ – Martin R Dec 14 '15 at 22:37

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.