[swift-evolution] [Review] SE-0023 API Design Guidelines

[Paul Cantrell]

I really like the general spirit of these guidelines. I particularly appreciate their emphasis on clarity at the point of use, and their recognition that both brevity and verbosity can be the enemy of clarity.

Some of the particulars of the guidelines haven’t worked well for me in practice — and I see from this thread that others have hit some of the same problems. The guidelines as they stand feel like they’ve mostly been vetted against a bunch of ADTs (which is probably true — stdlib?), and could use exposure to a broader range of libraries.

Immediately after the guidelines were published, before this review began, I tried applying them to Siesta. The project has 244 public declarations, of which I found 28 that the guidelines seemed to recommend changing. Of those 28, after much reflection and discussion, I ended up changing only 7 to match the guidelines (and also cleaning up several others, but in a non-guideline-compliant way).

In short, in a real-world project, pre-guidelines code agreed with the guidelines 89% of the time, but where it disagreed, the guidelines achieved only a 25% acceptance rate with the curmudgeonly developers.

You can follow that discussion here:

	https://gist.github.com/pcantrell/22a6564ca7d22789315b <https://gist.github.com/pcantrell/22a6564ca7d22789315b>
	https://github.com/bustoutsolutions/siesta/issues/15 <https://github.com/bustoutsolutions/siesta/issues/15>

Several places where we rejected the guidelines relate to the raging debate about first argument labels. Here’s a rundown of those particular cases. (Since this message will be a long one, I’ll share in a separate message some notes on the other places where we rejected the guidelines on questions other than the first arg label.)

Hopefully some more concrete examples can be useful in informing the discussion.

_____________________________

Quick context: there are things called resources. Resources can have observers. Observers are either self-owned or have an external owner object. Observers can either conform to a protocol or be closures; if the the latter, then they _must_ have an external owner (because closures aren’t objects).

There are thus three different methods to add observers to a resource — though they all cover the same underlying notion of “observer” (and in fact all boil down to the same thing internally):

    resource.addObserver(foo)

    resource.addObserver(fancyIndicator, owner: foo)

    resource.addObserver(owner: foo) {
        resource, event in
        updateStuff(resource.latestData)
    }

The API guidelines as stated would have us change that last one to:

    resource.addObserverWithOwner(foo) {
        resource, event in
        updateStuff(resource.latestData)
    }

However, this incorrectly implies that there is a distinct kind of thing that is an “observer with owner,” and that we will get one only from the third flavor of the method. That implication is wrong.

The consistency in the original of the “addObserver” name and the “owner:” label make the correct implication: all three methods serve the same purpose, observers are observers, and “owner” means the same thing in the two places it appears. I certainly think the non-compliant version of the code reads better.

There was extensive discussion around another family of methods that return a resource given either a path fragment or an absolute URL. This is where we ended up:

    service.resource("/foo")
    service.resource(absoluteURL: "http://bar.com")
    service.resource(absoluteURL: NSURL(string: "http://bar.com"))

(The first is by far the most common, the “standard” flavor.)

The guidelines would have had us do this:

    service.resourceWithPathFragment("/foo")
    service.resourceWithAbsoluteURL("http://bar.com")
    service.resourceWithAbsoluteURL(NSURL(string: "http://bar.com"))

To my eyes, this crosses the line into verbosity that impedes clarity, but there’s an even more serious problem: it wrongly implies that there’s a distinction between “resources with path fragments” and “resources with absolute URLs.” That’s dangerously wrong. One of the central conceits of the whole library is that _all_ resources get a canonicalized absolute URL, and there’s a uniqueness guarantee for that URL no matter whether it was constructed for a path fragment, an absolute URL, or resource-relative navigation.

In the cases of both addObserver(…) and service.resource(…), the guidelines would have us actively mislead users.

Another trickier example of the same issue is the much-discussed typedContent method, which downcasts based on inferred type and returns a default value if content is either missing or of the wrong type:

    var image: UIImage?
    ...
    image = imageResource.typedContent(ifNone: placeholderImage)

How to make this conform to the guidelines? The obvious fix is terrible:

    image = imageResource.typedContentIfNone(placeholderImage)

This implies … what? That the method only returns typed content if there is none of the placeholder image? No, clearly a rephrasing is necessary:

    image = imageResource.typedContentWithDefault(placeholderImage)

But again we have this problem of determining whether “with default” describes the method’s behavior, its result, or its first argument. Are we attaching content with the default somehow attached to it? (No.) Are we returning some content, or a default value if there is none? (Yes.) So maybe this is better:

    image = imageResource.typedContentOrDefault(placeholderImage)

But now my brain is having parsing problems. What is the LHS of that “or?” It just doesn’t read naturally. OK, maybe even more words can save us:

    image = imageResource.typedContentOrDefaultIfNone(placeholderImage)

Yuck. At this point, we might as well stuff the entire method abstract in the name:

    image = imageResource.typedContentOrDefaultIfNoneOrMismatchedType(placeholderImage)

Yuck squared. The original is so much clearer:

    image = imageResource.typedContent(ifNone: placeholderImage)

IMO, there’s nothing wrong with leaning on programming language syntax to help segment and clarify English syntax.

_______________________________

What’s the better guideline on first argument labels?

Radek had a nice thought in the aforementioned Gihub thread:

> The rationale being, ifNone doesn't really describe the method … it describes the parameter. Most of the time, the job of the method makes the first parameter obvious (hence the guideline), but here, it doesn't. So the parameter makes sense.


I’ll give a +1 for these two recommendations from Erica, which run along the same lines as Radek’s thought, but in more thorough detail:

> On Jan 23, 2016, at 6:33 PM, Erica Sadun via swift-evolution <swift-evolution at swift.org> wrote:
> 
> Prefer external names for the first parameter when the natural
> semantic relationship between the parameters is stronger than their
> relation to the operation. 
> 
> For example, the following calls use labels for the first parameter:
> 
> login(userName: "blah", password: "...")
> moveTo(x: 50.0, y: 30.0)
> 
> This example is contrary to Swift's normal naming scheme which integrates the
> first argument into the function or method name, for example:
> 
> loginWithUserName("blah", password: "...")
> moveToX(50.0, y: 30.0)
> 
> The coupling between x and y, username and password, (and yes it is a judgement call) 
> should be considered as a reason to employ an external label.
…
> 
> Differentiate related calls whose implementations are distinguished by their
> parameters, as you would with initializers, using first parameter labels.
> 
> Instead of loginWithUserName("blah", password: "...") and loginWithCredential(myCredential),
> prefer:
> 
> login(userName: "blah", password: "...")
> login(credential: myCredential)

I’m not sure we’ve found the perfect, crisp way of saying all this — but I strongly agree that the existing guidelines are too rigid on the question of the first arg label, and Erica’s wording comes the closest I’ve seen to being a viable replacement.

Cheers,

Paul

-------------- next part --------------
An HTML attachment was scrubbed...
URL: <https://lists.swift.org/pipermail/swift-evolution/attachments/20160124/8c439111/attachment.html>