4
\$\begingroup\$

I have a class, which is meant to show a full screen modal controller with a set of actions (apart from actions it also has some static content, a lot of labels and images which are irrelevant, so I omitted approximately 500 lines of code to keep it simple). It looks as follows:

import UI_Framework

class DescriptionViewController: UIViewController {

    var actions: [(title: String, action: () -> Void)]! {
        didSet {
            setUpActions(actions)
        }
    }

    private var actionStackView: UIStackView!

    // MARK: Lifecycle

    init(actions: [(title: String, action: () -> Void)]? = nil) {
        super.init(nibName: nil, bundle: nil)
        if let actions = actions {
            self.actions = actions
        } else {
            self.actions = [(title: "Close", action: { [weak self] in
                 guard let strongSelf = self else {
                     return
                 }
                 strongSelf.dismiss(animated: true)
            }]
        }
    }

    func viewDidLoad() {
        super.viewDidLoad()
        actionStackView = UIStackView(axis: .horizontal, spacing: 20)
        view.addSubview(actionStackView)
        actionStackView.fillSuperView()
        setUpActions(actions)
    }

    // MARK: Private

    private func setUpActions(_ actions: [(title: String, action: () -> Void)]) {
        guard let actionStackView = actionStackView else {
            return
        }

        actionStackView.subviews.forEach { $0.removeFromSuperview() }
        actions.map { CustomButton(title: $0.title, onClick: $0.action) }.forEach { actionStackView.addArrangedSubview($0) }
    }

}

Important note - I cannot define default actions during first phase of initialization because these actions are actually referencing to the class instace itself.

The idea behind having actions property force unwrapped instead of optional is because it's actually not supposed to have a nil value, as it may get somewhat confusing: what is meant by nil? Empty array of actions or default actions? It's especially crucial in the constructor, as it can take any of the values and should treat them differently. So the force-unwrapped property explains it by itself - do not make me nil, set empty array if you want empty array, keep it explicit and clear. Even if anyone sets it to nil by mistake it should get easily discovered - app will just crash.

However there is an opinion that force-unwrapping should be avoided at any cost to not introduce an error-prone code which might crash the app.

What is the better approach here?

\$\endgroup\$
  • \$\begingroup\$ @200_success, hey folks, I edited my code snippet in accordance with your complaints \$\endgroup\$ – The Dreams Wind May 24 at 9:24
  • \$\begingroup\$ You have explained why it is written the way it is, but what does it do? \$\endgroup\$ – pacmaninbw May 24 at 10:32
  • \$\begingroup\$ @pacmaninbw it's just a modal view controller supposed to provide some description with arbitrary set of action items \$\endgroup\$ – The Dreams Wind May 24 at 11:27
3
\$\begingroup\$

A couple of observations:

  1. You said:

    ... there is an opinion that force-unwrapping should be avoided at any cost to not introduce an error-prone code which might crash the app.

    Obviously, “at any cost” is too strong. If it was to be avoided at all costs, it wouldn’t be part of the language. But avoid it if you reasonably can. But we use it all the time (e.g. @IBOutlet references). So use it where appropriate, but avoid it if you can (without contorting yourself too much).

  2. You go on to say:

    The idea behind having actions property force unwrapped instead of optional is because it’s actually not supposed to have a nil value, as it may get somewhat confusing: what is meant by nil?

    I’d buy that argument if you didn’t then have a constructor that accepted nil and then supplied that default “close” action.

Personally, I’d embrace that it is optional and then have setUpActions handle the nil and empty scenarios.

Thus, perhaps:

public struct Action {
    let title: String
    let action: (() -> Void)?

    init(title: String, action: (() -> Void)? = nil) {
        self.title = title
        self.action = action
    }
}

class DescriptionViewController: UIViewController {
    private var actions: [Action]? = nil

    // MARK: Lifecycle

    init(actions: [Action]? = nil) {
        self.actions = actions

        super.init(nibName: nil, bundle: nil)
    }

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

    override func viewDidLoad() {
        super.viewDidLoad()

        let actionStackView = UIStackView(axis: .horizontal, spacing: 20)
        view.addSubview(actionStackView)
        actionStackView.fillSuperView()
        setUpActions(for: actionStackView)
    }

    func addAction(_ action: Action) {
        if actions == nil { actions = [] }
        actions!.append(action)
    }
}

// MARK: - Private methods

private extension DescriptionViewController {
    func setUpActions(for stackView: UIStackView) {
        if actions?.isEmpty ?? true {
            actions = [Action(title: "Close")]
        }

        actions?.map { action in
            CustomButton(title: action.title) { [weak self] in
                    action.action?()
                    self?.dismiss(animated: true)
                }
            }
            .forEach { stackView.addArrangedSubview($0) }
    }
}

A few observations in the above:

  • Note, I’ve replaced the named tuple scenario with a custom type, Action. That seems better than repeating the tuple definition repeatedly.

  • I’ve also changed it to add that default “close” action if nil or isEmpty, though if you want it to only do that in the nil scenario, that’s a simple change, too.

  • You don’t need that strongSelf pattern in the “close” closure. self?.dismiss(...) achieves the same thing. Besides, if you did need that strongSelf pattern, since Swift 4.2, you’d just do:

    guard let self = self else { ... }
    

    See SE-0079.

  • I’ve deferred the setUpActions until viewDidLoad, permitting a flow where you initialize it without any actions and then subsequently set the actions (but presumably still before you present it).

    I’ve also simplified this, eliminating this concept of adding actions after the view controller has already been presented. Go ahead and keep that stuff if you need it, but it has a hint of code smell.

    For example, in what situation will you have this presented and then subsequently add new actions. That might not be a great UX (where the user may be in the processing of trying to tap the screen and then everything shifts around as new buttons are added, possibly resulting on a tap on something other than what they originally intended).

  • As part of this refactoring, I’ve also removed the observer on actions. In general, one should hesitate to add observer to a collection. E.g. what if the app developer is adding one button at a time to the array. By the time they add the fifth button, the setUpActions actions will have been called five times. Sure, you probably won’t do that, but it’s the sort of thing that bites you later on, when you forget what’s going on. Admittedly, there are likely so few items that it matters, but as a general rule, be careful with observers on collections. Besides, it’s better to defer the building of UIKit controls until viewDidLoad.

  • I’ve tweaked this so that the setUpActions routine adds the self?.dismiss(animated: true) itself. That way, when the app developer adds their own actions, they don’t have to worry about dismissing the DescriptionViewController itself, but rather every button will, after running the supplied closure (if any) will automatically, dismiss the DescriptionViewController. Then, you can do things like:

    let actionSheet = DescriptionViewController()
    actionSheet.addAction(Action(title: "Save") {
        // put save logic here
    })
    
    actionSheet.addAction(Action(title: "Cancel")) // no closure needed as we don't need to do anything
    

    We can avoid the cruft of dismissing the DescriptionViewController from every added action.

  • By the way, you’ve said that you’ve omitted some 500 lines of code. Hopefully that’s an exaggeration or much of that code isn’t actually in the view controller (e.g. the UIStackView extension, the CustomButton, the UIView extension, etc.). But if you are saying that there are 500 more lines in the view controller, you might want to think creatively about refactoring that. (E.g. in the spirit of Dave DeLong’s A Better MVC.)

\$\endgroup\$
  • \$\begingroup\$ Thanks for this extensive answer. However, wouldn't it be inconsistent to allow setting the variable nil which eventually turns into default values instead of nil? I opted to have it in constructor only so I don't need to explain the same peculiarities in two places. \$\endgroup\$ – The Dreams Wind May 27 at 16:31
  • \$\begingroup\$ Yes, I wouldn’t advise it at all, but I was trying to accommodate this strange requirement. I’d personally follow the pattern of UIAlertController, with no default buttons. But the idea of default set by initializer doesn’t feel right. E.g. let controller = DescriptionViewController(); controller.actions?.append(Action(title: "Save") { ... }). Is it reasonable for the app developer to think that there are now two buttons after that? That’s why I’d defer the default button logic until later to catch edge case where app developer didn’t specify any (or omit it entirely). \$\endgroup\$ – Rob May 27 at 16:43
  • \$\begingroup\$ By the way, I’ve tweaked the above so that the application developer doesn’t have to add self?.dismiss(animated: true) to every action they add to the DescriptionViewController. I’ve also made actions private and supplied an addAction method. \$\endgroup\$ – Rob May 27 at 17:29
  • \$\begingroup\$ thanks! I should say that I would rather accept your comment as a correct answer, as I ended up with the same thoughts - consistency is more important than convenience in this case. Thanks for all your input here \$\endgroup\$ – The Dreams Wind May 28 at 11:27

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.