1
\$\begingroup\$

I've got several UIViewControllers that are trying to do too much so I'm trying to learn how to clean them up so that they only have code that does UI things (ultimately getting things to conform to MVVM).

I know my code needs a lot of improvement so I'm going to start small by focusing on one particular behavior to ask for help with which is saving the user's selection as a user default, but keeping the code that does this out of the UIViewController.

In this example from my app, I have a UITableViewController called FormulasTableViewController which has a list of the formulas the app can use for calculations to choose from:

class FormulasTableViewController: UITableViewController {

  let formulas: [CalculationFormula] = [
    CalculationFormula.epley,
    CalculationFormula.baechle,
    CalculationFormula.brzychi,
    CalculationFormula.lander,
    CalculationFormula.lombardi,
    CalculationFormula.mayhewEtAl,
    CalculationFormula.oConnerEtAl]

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

    refresh()
  }

  override func viewDidLoad() {
    super.viewDidLoad()
  }

  // MARK: - Table view data source

  override func numberOfSectionsInTableView(tableView: UITableView) -> Int {
    return 1
  }

  override func tableView(tableView: UITableView, numberOfRowsInSection section: Int) -> Int {
    return formulas.count
  }

  override func tableView(tableView: UITableView, cellForRowAtIndexPath indexPath: NSIndexPath) -> UITableViewCell {
    let cell = tableView.dequeueReusableCellWithIdentifier("formulasCell", forIndexPath: indexPath)
    let currentFormula = formulas[indexPath.row]
    cell.textLabel?.text = currentFormula.rawValue
    return cell
  }

  func refresh() {
    let preferredFormula = UserDefaultsManager.sharedInstance.preferredFormula
    for index in 0 ..< formulas.count {
      let indexPath = NSIndexPath(forItem: index, inSection: 0)
      let currentFormula = formulas[indexPath.row]
      if let cell = tableView.cellForRowAtIndexPath(indexPath) {
        cell.accessoryType = preferredFormula! == currentFormula ? .Checkmark : .None
      }
    }
  }

  override func tableView(tableView: UITableView, didSelectRowAtIndexPath indexPath: NSIndexPath) {
    tableView.deselectRowAtIndexPath(indexPath, animated: true)

    UserDefaultsManager.sharedInstance.preferredFormula = formulas[indexPath.row]

    refresh()
  }
}

As you can see above in tableView(tableView:didSelectRowAtIndexPath:indexPath), when the user selects one, UserDefaultsManager.sharedInstance.preferredFormula = formulas[indexPath.row] is executed.

class UserDefaultsManager {
  static let sharedInstance = UserDefaultsManager()

  // MARK: Preferred Formula
  private var defaultsFormula: CalculationFormula?

  var preferredFormula: CalculationFormula? {
    get {
      if self.defaultsFormula == nil {
        let defaults = NSUserDefaults.standardUserDefaults()
        if let defaultValue = defaults.objectForKey("preferredFormula") as? String {
          self.defaultsFormula = CalculationFormula(rawValue: defaultValue)
        } else {
          self.defaultsFormula = CalculationFormula(rawValue: "Baechle")
        }
      }
      return self.defaultsFormula
    }

    set {
      self.defaultsFormula = newValue
      let defaults = NSUserDefaults.standardUserDefaults()
      if (self.defaultsFormula == nil) {
        defaults.removeObjectForKey("preferredFormula")
      } else {
        defaults.setObject(self.defaultsFormula!.rawValue, forKey: "preferredFormula")
      }
    }
  }
}

My thinking here is that because selection of a formula is happening in a table which is controlled by UIFormulasTableViewController, UserDefaultsManager.sharedInstance.preferredFormula = formulas[indexPath.row] shouldn't be here.

I think I want to use delegation so that my UserDefaultsManager class will handle the user selection of a table row. But my UserDefaultsManager class is handling a lot of different default settings so if I create a protocol on all the different UIViewControllers that are similar to my UIFormulasTableViewController so UserDefaultsManager can act as their delegate, it's going to conform to a long list of these protocols and that seems silly - but I'm a beginner, so maybe it's not.

To remove this code from the UIViewController would it be reasonable to take this delegation approach?

\$\endgroup\$
  • \$\begingroup\$ Please do not update the code in your question to incorporate feedback from answers, doing so goes against the Question + Answer style of Code Review. This is not a forum where you should keep the most updated version in your question. Please see what you may and may not do after receiving answers. \$\endgroup\$ – Mast May 11 '17 at 13:44
  • \$\begingroup\$ Sorry, Mast. I didn't realize that wasn't allowed. I'm used to editing a question on Stack Overflow but I see why that style isn't desired. \$\endgroup\$ – Jim May 11 '17 at 13:53
2
\$\begingroup\$

For me it's a bit misleading that you have CalculationFormula all over the place. I'd create an extra model or struct to handle the formula logic or just hold the data in memory.

To persist the data using UserDefaults is okay, but in your UserDefaultsManager you juggle too much with CalculationFormulas. For example, as a reader I don't understand the intention to name an optional property defaultsFormula when it does not default to anything because it can be nil. So the question is if the default formula should be dynamically set or would it be enough to provide a static default? After this decision, move it over to the model/struct mentioned above. Your preferredForumla property becomes simpler then.

Also move the collection of CalculationFormulas from your view controller to the mentioned file.

With the new class/struct you can make use of the delegation approach.

\$\endgroup\$
  • \$\begingroup\$ Hi Marc, Thanks for taking the time to look at this. Your points are all spot on. I’ve learned a lot and changed a lot of code since I posted this. I’d love your feedback on the changes that I’ve as a new answer below. \$\endgroup\$ – Jim May 11 '17 at 13:50
0
\$\begingroup\$

Here are the changes I ended up making.

I abandoned the idea of using an enum for the formulas. I already had the formulas as NSManagedObjects and now I know how to get what I need with those. I also became acquainted with NSFetchedResultsController which is perfect for this view and what I need to do.

Now, when the user selects one of the formulas, here’s what happens:

  override func tableView(_ tableView: UITableView, didSelectRowAt indexPath: IndexPath) {
    tableView.deselectRow(at: indexPath, animated: true)

    let selectedFormula = viewModel.fetchedResultsController.object(at: indexPath) as! Formula
    let newDefaultFormulaUuid = selectedFormula.uuid

    viewModel.onNewFormulaSelected(newDefaultFormulaUuid)

    refresh()
  }

the new formula uuid gets passed to this method in the view model where the new default formula uuid is set as the new default formula uuid:

func onNewFormulaSelected(_ newFormulaUuid: String) {
    UserDefaults.setDefaultFormulaUuid(newFormulaUuid)

    modelLiftEvent.updateLiftEventFormula(newFormulaUuid)
}

The getting and setting of the default formula uuid now is much simpler:

// MARK: Default Formula

static func formula() -> String {
    let storedValue = UserDefaults.standard.string(forKey: UserDefaultsKeys.formulaUuid)
    return storedValue ?? "378E6A58-3A87-4B71-B04A-91106ED8C717" // Epley
}

static func setDefaultFormulaUuid(_ formulaUuid: String) {
    standard.setValue(formulaUuid, forKey: UserDefaultsKeys.formulaUuid)
}

A lot cleaner, I'd say.

\$\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.