Delegate methods

[zenangst]

It got a bit inspired by http://khanlou.com/2016/09/swifty-delegates/ that @onmyway133 shared in the previous discussion. So I wanted to run some stuff by you when building delegate methods.

I would propose that we start moving towards what the article is describing.
To give you a concrete example from the Spots Swift 3 migration.
I'd like to change the public API for the SpotsDelegate method that gets invoked whenever someone taps on an item.

Before

func spotDidSelectItem(_ spot: Spotable, item: Item)

After

func didSelect(item: Item, in spot: Spotable)

I think that reads much nicer.

What do you guys think? @hyperoslo/ios

[vadymmarkov]

It will take time to get used to it, but I like the idea.

[onmyway133]

This reads better. But I think you mean

func didSelect(item: Item, in spot: Spotable)

[zenangst]

@onmyway133 correct :) I'll fix it!

[vadymmarkov]

Not sure about barcode scanner for example. Now we have this:

public protocol BarcodeScannerCodeDelegate: class {
  func barcodeScanner(_ controller: BarcodeScannerController, didCaptureCode code: String, type: String)
}

public protocol BarcodeScannerDismissalDelegate: class {
  func barcodeScannerDidDismiss(_ controller: BarcodeScannerController)
}

Should it be changed to:

public protocol BarcodeScannerCodeDelegate: class {
  func didCapture(code: String, type: String, in controller: BarcodeScannerController)
}

public protocol BarcodeScannerDismissalDelegate: class {
  func didDismiss(controller: BarcodeScannerController)
}

func didDismiss(controller: BarcodeScannerController) could cause conflicts:

[zenangst]

Oh I see, can you change the label to call it something else than controller would that help?

public protocol BarcodeScannerDismissalDelegate: class {
  func didDismiss(barcodeScanner: BarcodeScannerController)
}

[vadymmarkov]

@zenangst It will solve the problem, but I still find this naming a bit confusing. For example:

// Old way:

protocol AdminHeaderViewDelegate: class {
  func adminHeaderViewDidExpand(_ adminHeaderView: AdminHeaderView)
}

delegate?.adminHeaderViewDidExpand(self)

// New way, option 1:

protocol AdminHeaderViewDelegate: class {
  func didExpand(adminHeaderView: AdminHeaderView)
}

delegate?.didExpand(adminHeaderView: self)

// New way, option 2
// Those 2 delegate methods make more sense, but are conflicting for compiler:

protocol AdminHeaderViewDelegate: class {
  func didExpand(in adminHeaderView: AdminHeaderView)
}

protocol AdminFooterViewDelegate: class {
  func didExpand(in adminFooterView: AdminFooterView)
}

Also when I type did in the controller I get a lot of methods not really related to delegates or to my AdminHeaderViewDelegate specifically:

[RamonGilabert]

I like New way, option 1, I don't need the in label in this case.

[vadymmarkov]

Also in the example above it's AdminHeaderView who makes an action, not the delegate which is more like observer. So as for me it makes more sense to have adminHeaderView prefix in delegate function names.

[JohnSundell]

Really interesting idea! 🤔 But I agree with @vadymmarkov, it does make it a bit hard to find the API that you're looking for. It also reduces API discoverability and makes it harder to know where a method "comes from" when implemented. Finally, it would make our code not inline with Apple's naming conventions (if this is something that's important to us, which at least for Open Source, I personally think it should be) 😅

The nice thing about always using the class name as a prefix for the delegate method names is that things are crystal clear. There are also ways to make the APIs nicer without removing the prefix naming. In the example given in the first post func spotDidSelectItem(_ spot: Spotable, item: Item), we could make it nicer (and more "swifty"), by changing it to this:

func spot(_ spot: Spotable, itemSelected item: Item)

What do you guys think about that? 😄

[zenangst]

I like this, we should update the public API's for frameworks such as Spots to be more scope with relevant prefixes. I guess that could go into the 6.0.0 release. What do you guys think?

[zenangst]

The methods in question here ended up looking like this:

func spotable(_ spot: Spotable, itemSelected item: Item)
func spotablesDidChange(_ spots: [Spotable])
func spotable(_ spot: Spotable, willDisplay view: SpotView, item: Item)
func spotable(_ spot: Spotable, didEndDisplaying view: SpotView, item: Item)