There are quite a few issues here:
In switch statement, you don’t need/want break statements. Unlike C languages, Swift switch statements don’t fallthrough by default. To use break where it is not needed is very unswifty.
I’d make toggleItem an instance method.
The properties of the ViewController should not be static.
This table view controller should not be reaching into ViewController to update its properties, which tightly couples these two classes. We should strive for far looser coupling in our code.
Admittedly, because ViewController is presenting TableViewController, the presenting view controller might go ahead and update properties in the presented view controller, in order to pass data to down the chain. But TableViewController should never be reaching back up the chain. That makes them too tightly coupled and only allows TableViewController to be called from that one view controller, whereas as your app grows, you might want to call it from elsewhere.
The typical solution to achieve loose coupling is the delegate-protocol pattern. Presented view controllers should not updating properties in the presenting view controller, but rather it should only communicate back via its protocol. This allows us to use this presented view controller from wherever we want.
If you’re going to have an early exit in didSelectRowAt (if section ≠ 0), all things being equal, you should prefer guard over if, as it makes the intent more clear (you see guard keyword and you immediately know “oh, this is an early exit for some edge case”). The compiler will also warn us if we accidentally forget to call return.
In if statements, the use of parentheses around the test clause feels a bit unswifty. Consider:
if (state == PastState.UNSELECTED) {
...
} else {
...
}
Instead, I’d suggest:
if state == PastState.UNSELECTED {
...
} else {
...
}
In the case of an enumeration, I’d favor switch over if. Thus, instead of the above, I’d suggest:
switch state {
case PastState.UNSELECTED:
...
case PastState.SELECTED:
...
}
This ensures that the test is exhaustive. It makes it easier to reason about the code at a glance. Plus, if you, for example, later add a .NOTDETERMINED enumeration, the compiler can warn you of all of the non-exhaustive switch statements, but if you use if-else, you have to manually pour through all of your code looking for this yourself.
Because PastState is an enumeration, you can eliminate all of those redundant references to the enumeration name. E.g., using the previous example, you can say:
switch state {
case .UNSELECTED:
...
case .SELECTED:
...
}
In your enumerations, you should use camelCase (start with lower case letter, only capitalize start of new word within the case name):
switch state {
case .unselected:
...
case .selected:
...
}
Alternatively, just retire this enumeration altogether and just use a [Bool] property. Or, as shown at the end of this answer, if you use a view model, you might refactor this out completely.
The blacklisted property is using an enumeration, which is good. But flags is using strings with values buried in the code. They both should be enumerations.
The done method is subtracting blacklisted from the ViewController’s allowed. But what if you unselected a blacklisted item and needed to add it back to the allowed? The existing code won’t do that.
The same issue applies for ViewController.flags, where you’re adding flags. But what if one was removed?
You are setting the accessoryView based upon which flags/blacklisted values exist in these sets and toggle this based upon selecting a row. But you then have this UISwitch, too. You’d generally use either a UISwitch or the accessoryView, but not both. Pick one or the other. (In my demo, I’m going to retire UISwitch.)
The references to self.dismiss(...) can be simplified to just dismiss(...).
The didSelectRowAt is:
- toggling the
indicatorView based upon the old state;
- updating the
blacklist/flags properties; and
- toggling the
state.
I would suggest that you simply want to toggle the state and then reload that row. Not only does it make this less fragile (you don’t have three different operations that you need to carefully coordinate), but it affords the opportunity to animate the change of the indicatorView.
I would suggest grouping your view controller methods into extensions. For example, I find it nice to put UITableViewDataSource methods in one extension, UITableViewDelegate methods in another, @IBAction methods in another, etc. This makes it easy to visually identify/find relevant methods. This also allows you to collapse sections (e.g. ⌘+option+◀︎) sections that you’re not currently working on, making it easy to focus on the work at hand. This, combined with use of the MARK: - designator makes navigation through your view controller easier.
You’ve got an enumeration called PastState. But this is not the “past state”, but rather the current state as updated as the user proceeds with selecting/unselecting various options. So I might rename that SelectedState or something like that.
Likewise, I might give TableViewController a name that indicates its purpose. E.g. maybe AllowedContentViewController.
I find it confusing that the main view controller uses allowed, but the TableViewController uses blacklisted (which is presumably an inverted set of the aforementioned). I’d suggest using the same model in both view controllers. If TableViewController wants to invert it as part of its determination of what’s checked and what’s not, then it should do that.
The most radical change I’m going to suggest is that you should completely remove the business logic from the view controller. The view controller should be handling the interface with the UIKit controls (the table view, the buttons, etc.), but all the logic about which flags and Category values is represented by which row, how you toggle them, etc., doesn’t belong here. You often put it in what I will call a “view model” (though this entity goes by different names)
The virtue of this is two fold. First the view controller suddenly becomes something very concise and easy to understand. Second, when you get around to writing unit tests, you want to be able to test your business logic without worrying about UIKit stuff (which should be part of the UI tests, not the unit tests).
So, pulling this all together, we end up with a view controller like this:
class AllowedContentViewController: UITableViewController {
// properties set by presenting view controller
weak var delegate: AllowedContentViewControllerDelegate?
var allowed: Set<Category>!
var flags: Set<Flag>!
// properties used by this view controller
private var viewModel: AllowedContentViewModel!
override func viewDidLoad() {
super.viewDidLoad()
viewModel = AllowedContentViewModel(allowed: allowed, flags: flags)
}
}
// MARK: - Actions
extension AllowedContentViewController {
@IBAction func cancel(_ sender: UIBarButtonItem) {
dismiss(animated: true)
}
@IBAction func done(_ sender: UIBarButtonItem) {
delegate?.allowedContentViewController(self, didUpdateAllowed: viewModel.allowed, flags: viewModel.flags)
dismiss(animated: true)
}
}
// MARK: - UITableViewDataSource
extension AllowedContentViewController {
// you didn't provide the following two, so I'll take a guess what that looked like
override func tableView(_ tableView: UITableView, numberOfRowsInSection section: Int) -> Int {
return viewModel.numberOfRows()
}
override func tableView(_ tableView: UITableView, cellForRowAt indexPath: IndexPath) -> UITableViewCell {
let cell = tableView.dequeueReusableCell(withIdentifier: "Cell", for: indexPath)
cell.textLabel?.text = viewModel.text(for: indexPath.row)
cell.accessoryType = viewModel.isSelected(row: indexPath.row) ? .checkmark : .none
return cell
}
}
// MARK: - UITableViewDelegate
extension AllowedContentViewController {
override func tableView(_ tableView: UITableView, didSelectRowAt indexPath: IndexPath) {
guard indexPath.section == 0 else {
return
}
viewModel.toggle(row: indexPath.row)
tableView.reloadRows(at: [indexPath], with: .fade)
}
}
That references a protocol to which our presenting view controller will conform:
protocol AllowedContentViewControllerDelegate: class {
func allowedContentViewController(_ viewController: AllowedContentViewController, didUpdateAllowed allowed: Set<Category>, flags: Set<Flag>)
}
The presenting view controller would then do something like so:
class ViewController: UIViewController {
var allowed = Set<Category>()
var flags = Set<Flag>()
override func prepare(for segue: UIStoryboardSegue, sender: Any?) {
if let destination = (segue.destination as? UINavigationController)?.topViewController as? AllowedContentViewController {
destination.flags = flags
destination.allowed = allowed
destination.delegate = self
}
}
}
extension ViewController: AllowedContentViewControllerDelegate {
func allowedContentViewController(_ viewController: AllowedContentViewController, didUpdateAllowed allowed: Set<Category>, flags: Set<Flag>) {
self.allowed = allowed
self.flags = flags
}
}
I obviously made an assumption here of how you presented this AllowedContentViewController (namely that you must have embedded it in a UINavigationController, based upon the presence of dismiss but that you also had @IBActions with UIBarButton). Clearly, adjust the prepare(for:sender:) according to how you actually presented this table view controller.
Anyway, above, the presenting view controller is responsible for passing data to this presented AllowedContentViewController and for responding to AllowedContentViewControllerDelegate delegate method that supplies the updated user preferences for what is allowed and what isn’t. This gets the AllowedContentViewController from ever having to reach into ViewController to retrieve/update properties itself. Now AllowedContentViewController can be used in any context you want.
Finally, our view model has all the business logic that used to be in the TableViewController:
struct AllowedContentViewModel {
var allowed: Set<Category>
var flags: Set<Flag>
func numberOfRows() -> Int {
return 4
}
func isSelected(row: Int) -> Bool {
switch row {
case 0: return flags.contains(.nsfw)
case 1: return flags.contains(.religious) && flags.contains(.political)
case 2: return !allowed.contains(.programming)
case 3: return !allowed.contains(.dark)
default: fatalError("Unexpected row number")
}
}
private mutating func select(row: Int) {
switch row {
case 0: flags.insert(.nsfw)
case 1: flags.insert(.religious); flags.insert(.political)
case 2: allowed.remove(.programming)
case 3: allowed.remove(.dark)
default: fatalError("Unexpected row number")
}
}
private mutating func unselect(row: Int) {
switch row {
case 0: flags.remove(.nsfw)
case 1: flags.remove(.religious); flags.remove(.political)
case 2: allowed.insert(.programming)
case 3: allowed.insert(.dark)
default: fatalError("Unexpected row number")
}
}
mutating func toggle(row: Int) {
if isSelected(row: row) {
unselect(row: row)
} else {
select(row: row)
}
}
func text(for row: Int) -> String {
switch row {
case 0: return NSLocalizedString("NSFW", comment: "Label")
case 1: return NSLocalizedString("Religious/political", comment: "Label")
case 2: return NSLocalizedString("Blacklisted Programming", comment: "Label")
case 3: return NSLocalizedString("Blacklisted Dark", comment: "Label")
default: fatalError("Unexpected row number")
}
}
}
We can, if we want, then write unit tests for this view model. For example:
class AllowedContentViewModelTests: XCTestCase {
func testDefaultState() {
let viewModel = AllowedContentViewModel(allowed: [], flags: [])
XCTAssert(!viewModel.isSelected(row: 0))
XCTAssert(!viewModel.isSelected(row: 1))
XCTAssert(viewModel.isSelected(row: 2))
XCTAssert(viewModel.isSelected(row: 3))
}
func testTogglingNsfw() {
var viewModel = AllowedContentViewModel(allowed: [], flags: [.nsfw])
XCTAssert(viewModel.isSelected(row: 0))
viewModel.toggle(row: 0)
XCTAssert(!viewModel.isSelected(row: 0))
}
}
See example at https://github.com/robertmryan/RefactoredViewController.
.swiftlint.ymlthat I use to tailor it to my personal programing needs, but you should feel free to tweak to your personal preferences. But tools like this can be especially helpful if you’re new to Swift and want to follow established conventions. \$\endgroup\$