Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Protocol conformance #116

Closed
ghost opened this issue Nov 23, 2015 · 11 comments
Closed

Protocol conformance #116

ghost opened this issue Nov 23, 2015 · 11 comments
Assignees

Comments

@ghost
Copy link

ghost commented Nov 23, 2015

The guides states:

When adding protocol conformance to a class, prefer adding a separate class extension for the protocol methods.

Does this also apply to instances where I'm indirectly adopting a protocol, say when a superclass adopts a protocol and I want to override the default implementation of the superclass?

@designatednerd
Copy link
Member

I've been doing it that way at work. I think for more experienced programmers it's more obvious, but for noobs it's waaaaaay less obvious, especially since you can't redeclare protocol conformance so you can click through to the protocol itself.

Ex:

class BaseTableViewDataSource: NSObject {
    //Do classy things
}

//MARK: - UITableViewDataSource

extension BaseTableViewDataSource: UITableViewDataSource {

    func numberOfSectionsInTableView(tableView: UITableView) -> Int {
        return 1
    }
    //Other required UITVDS methods
}

has been a lot easier for someone new to Swift to understand than:

class SpecificTableViewDataSource: BaseTableViewDataSource {
    //Do subclassy things
}

//MARK: - UITableViewDataSource

//Can't redeclare protocol conformance or the compiler whines 

extension SpecificTableViewDataSource { 

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

    //Maybe more methods? Maybe not? Who knows? 
}

Because of this, I would say for our purposes, it might be better to have anything overriding a superclass's implementation with the rest of the class.

@ghost
Copy link
Author

ghost commented Nov 23, 2015

Thanks @designatednerd 👍

The example I was given is as follows:

// This class is from a third-party lib
class SomeViewController: UICollectionViewDatasource, UICollectionViewDelegate {

  // Rest of implementation...

  func numberOfSectionsInCollectionView(collectionView: UICollectionView) -> Int {
    return 0
  }

}
// This is the class the reader builds throughout the tutorial
class MyViewController: SomeViewController {

  // Rest of implementation...

  override func numberOfSectionsInCollectionView(collectionView: UICollectionView) -> Int {
    return someArray.count()
  }

}

Is this OK, or should it actually be like this?:

class MyViewController: SomeViewController {

  // Rest of implementation...

}

// MARK - UICollectionViewDataSource

extension MyViewController {

  override func numberOfSectionsInCollectionView(collectionView: UICollectionView) -> Int {
    return someArray.count()
  }

}

Thoughts?

@hollance
Copy link
Member

I like moving the collection view delegate stuff into its own extension. It makes it a bit clearer what different jobs this view controller does.

(Arguably, an even better approach would be to make a completely separate class for the delegate stuff. That way it reduces the amount of jobs the view controller is doing.)

@mitchellporter
Copy link

.@hollance is correct that the better approach would be to offload the delegate methods to another object, but I'd be lying if I said I implement this approach regularly. It's just too easy to leave it in the view controller.

@JRG-Developer
Copy link
Member

Regarding example from @designatednerd

class SpecificTableViewDataSource: BaseTableViewDataSource {
    //Do subclassy things
}

//MARK: - UITableViewDataSource

extension SpecificTableViewDataSource { 

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

    // ...
}

I think //MARK: - UITableViewDataSource makes it clear what's going on.

In general, I try to provide empty sections or very simple methods already included as part of the starter project for the tutorials that I write. This way, I can tell the reader "add this to the UITableViewDataSource extension, right below blahBlahBlah".

I think we should leave the rule as-is-- by adding "unless" conditions, it's harder to follow the guide, IMHO.

@JRG-Developer
Copy link
Member

@designatednerd

BTW, couldn't BaseTableViewDataSource just conform to UITableViewDataSource directly (i.e. not subclass NSObject)?

In this case, I'd agree that putting it in the body would be okay-- as it'd be the only thing it's doing.

I think the underlying issue we're trying to address here is breaking up large classes by putting functionality/purpose into extensions.

@mitchellporter
Copy link

BTW, couldn't SpecificTableViewDataSource just conform to UITableViewDataSource directly (i.e. not subclass NSObject)?

@JRG-Developer Your class needs to conform to the NSObjectProtocol in order to be a UITableViewDelegate. If you try creating a class that doesn't subclass NSObject and then try conforming to UITableViewDelegate it will throw the following error:

Type 'YourSwiftClass' does not conform to protocol 'NSObjectProtocol'

@JRG-Developer
Copy link
Member

@mitchellporter You're right, thanks for the correction. This will teach me to comment without running code in a playground first. ;]

The point I was trying to make:

If the sole purpose of BaseTableViewDataSource and its subclass of SpecificTableViewDataSource is to act as a table view data source, I'd say it's fine to put the protocol methods within the class body.

E.g.

class BaseTableViewDataSource: NSObject, UITableViewDataSource {

  func numberOfSectionsInTableView(tableView: UITableView) -> Int {
    return 1
  }
  // other required methods...
}

class SpecificTableViewDataSource: BaseTableViewDataSource {

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

  // Do subclassy things...
}

However, I don't think this is what the style guide is talking about here-- rather, I think we're talking about When adding protocol conformance to a class [that does other things], prefer adding a separate class extension for the protocol methods.

In this specific case, it doesn't "do other things"-- it's sole purpose is to be the data source.

I think the guide is fine as is here. After all, it does say prefer and not though shalt must do...this would be a permissible exception, IMHO. ;]

@mitchellporter
Copy link

@JRG-Developer I completely agree, I would use an extension in a UIViewController since it's usually going to have a lot more going on, but wouldn't use one in a class that solely exists just to conform to that single protocol.

@designatednerd
Copy link
Member

I'd agree it's definitely more helpful in a vanilla UIViewController subclass where you'd be declaring the conformance directly. Maybe it might be good to think of a direct subclass of UITableViewController, which we subclass directly way more often than we create separate data sources when creating examples.

Would it make sense to still have the extension stuff broken out like so:

class SomeTableViewController: UITableViewController {

  override func viewDidLoad() {
    super.viewDidLoad()
    //Other view lifecycley stuff...
}

//MARK: - UITableViewDataSource

extension SomeTableViewController {

  override func numberOfSectionsInTableView(tableView: UITableView) -> Int {
    return 1
  }
  // other required methods...
}

or just have the stuff overriding methods in the superclass:

class SomeTableViewController: UITableViewController {

  override func viewDidLoad() {
    super.viewDidLoad()
    //Other view lifecycley stuff...
  }

  override func numberOfSectionsInTableView(tableView: UITableView) -> Int {
    return 1
  }
  // other required methods...
}

Actually, typing that out made me think maybe the rule of thumb should be if it needs an override, then it doesn't need a separate extension, since it must be doing something defined in the superclass.

Does that make sense?

@rayfix
Copy link
Contributor

rayfix commented Apr 7, 2016

After thinking about this a while, I like @designatednerd rule of 👍 An end-user that just wants to override a couple of things from the base class shouldn't be burdened with all of the book keeping. That said, I think there is some taste involved. If you are dealing with pages and pages of code, anything you can do to keep things "chunkable" goes a long way for understanding.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

5 participants