ReactiveCocoa implementation of async function

I would like a review of the RAC implementation for the function setGist()
I've posted the code on GitHub for easier reading.

Description:

• You create an instance of GistService()

• let gistService = GistService()

• Then use public functions .resetGist() -> Bool and .setGist(string: String) -> SignalProducer

gistService.setGist(content: content!)
    .on(next: {
        app.userNotification.pushNotification(openURLAction: $0.URL) //this is an NSUserNotification
    })
    .on(error: print)
    .start()

The function uses the Gists API

```
import Cocoa
import ReactiveCocoa
import SwiftyJSON

enum ConnectionError: ErrorType {

case Bad(String)
case Worse(String)
case Terrible(String)
}

/**

• TODO: Add OAuth2 towards GitHub as a protocol
• TODO: Make it RAC
• TODO: Store gistID in NSUserDefaults

*/
public final class GistService: NSObject {

//MARK:- Properties
var userDefaults: NSUserDefaults
var gistAPIURL: NSURL
var gistID: String?

//MARK:- Initialisation

override init() {
userDefaults = NSUserDefaults.standardUserDefaults()
gistAPIURL = NSURL(string: "https://api.github.com/gists")!
super.init()
}

convenience init(apiURL: String) {
self.init()
self.gistAPIURL = NSURL(string: apiURL)!
}

//MARK:- Public API

func resetGist() -> Bool {
// userDefaults.removeObjectForKey("gistID")
self.gistID = nil

return !gistUpdate()
}

func setGist(content content: String,
isPublic: Bool = false, fileName: String = "Casted.swift") // defaults
-> SignalProducer {

return SignalProducer {sink, disp in

let gitHubHTTPBody = [
"description": "Generated with Cast (cast.lfaoro.com)",
"public": isPublic,
"files": [fileName: ["content": content]],
]

let request: NSMutableURLRequest

/**
- TODO: Add OAuth2 towards GitHub as a protocol
- TODO: Make it RAC
- TODO: Store gistID in NSUserDefaults
*/

The right way to mark to-dos in Swift is like this:

// TODO: Some note about what you need to do.

Per this Stack Overflow answer, it is the most likely variant that Apple will eventually build into Xcode to automatically generate notes for, but additionally, a script is provided in that answer for generating this warning in the meantime.

public final class GistService: NSObject

• It's great that this class is public. It's the only way to use our classes across frameworks... but none of the properties or methods of this class are marked as public. So realistically, the class really can't be used outside the framework it's in.

• Why is this class final? What's the specific problem with inheriting from this class?

• Why is this class inheriting from NSObject? That would make sense in Objective-C (where every class must inherit from NSObject), but I don't see what we're getting from NSObject here. (In fact, later we'll see that inheriting from NSObject actually causes us problems.)

• I'm not sure how much I like the name GistService. Look at the names of Apple classes. Are any of them "services"?

override init() {
    userDefaults = NSUserDefaults.standardUserDefaults()
    gistAPIURL = NSURL(string: "https://api.github.com/gists")!
    super.init()
}

convenience init(apiURL: String) {
    self.init()
    self.gistAPIURL = NSURL(string: apiURL)!
}

First of all, userDefaults is unused here and should be eliminated.

Second of all, I'm not sure how appropriate is to default to GitHub. I just don't know at all how appropriate that is. If someone using the library wants a more convenient way of instantiating with this (or any other) default base URL, they can always extend the class.

Third of all, strings are not URLs and should not be called as such.

Fourth, and absolutely most importantly, you've done this backwards. You have the wrong init method marked as convenience. I think that inheriting from NSObject prevents you from marking the zero-argument init has convenience (and if so, all the more reason not to inherit from NSObject).

The designated initializers should be the ones that take more arguments than anything else. The convenience initializers call those designated initializers. They are convenient because they take fewer arguments and assume default values for the arguments that the designated initializers require.

So, with all these things in mind... we should start with this init method:

required init(baseURL: NSURL) {
    self.gistAPIURL = baseURL
}

And I'd argue we don't want the zero-argument init at all. (Why this is required, I'll address in a minute.)

If we really want to provide a means of instantiating with the GitHub url, then that should look something like this:

class func gitHubGist() -> Self {
    let gitHubURL = NSURL(string: "https://api.github.com/gists")!
    return self.init(baseURL: gitHubURL)
}

We want to return Self so that this works with inheritance. And to do this properly, we have to call self.init. The only way this works with the Self return type is if the init we're calling is marked as required.

So now, if I want a gist of my own choosing, I use this approach:

let myGist = GistService(baseURL: fooBarGistServiceAPIURL)

And if I want GitHub's, I can conveniently just do this:

let gitHubGist = GistService.gitHubGist()

//MARK:- Public API

This is not how you make methods public. As I mentioned earlier, our class is public and can be seen outside of the framework... but none of its methods or properties are. If these methods are intended to be part of the API, we need to start by marking them with public.

But that's not enough. We should also use AppleDoc style comments so that without having to navigate to your website, or where ever you're keeping documentation, the user can get an overview of what this method or property is or does right there in Xcode.

I highly recommend you check out this NSHipster article on all of what you can do with AppleDocs, but in the meantime, here's a little preview:

The comments I provided in lines 5 through 12 generated the content for the pop-up box (which you get by holding Option and clicking the function name). This documentation crosses file and framework boundaries. It's the surest way to make sure your documentation is visible to the people actually using your code. It's comment visible if you're browsing the source and documentation available when you're using the API.

//    userDefaults.removeObjectForKey("gistID")

If this code is unused, remove it. Let source control manage the fact that it used to be there. If you don't remove it, some maintainer will. And if it doesn't need to be removed, it needs a comment explaining why it needs to stay.

StackExchange Code Review Q#101021, answer score: 4