/**
- 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.
//MARK:- Helper functions
func gistUpdate() -> Bool {
guard let _ = self.gistID else { return false }
return true
}
First, if these are "Helper" functions, they should almost certainly be private (they're probably not needed outside this file, are they?). But problems exist beyond the access level of this method.
What's it even doing? It's certainly not updating anything.
If I only see the method signature, I see this:
func gistUpdate() -> Bool
And that's about all I see when I come across it in code (where your library uses it).
So, let's look at the actual implementation:
guard let _ = self.gistID else { return false }
return true
This is arguably the most confusing way of writing:
return self.gistID != nil
But this is a really bad method. We don't need a method for this. We need optional binding. If you're not comfortable with optional binding, it's definitely something you need to work on before you venture into anything more complicated in Swift.
So let's take a closer look at where you're calling this method. Here's a place:
if self.gistUpdate() {
let updateURL = NSURL(string: self.gistAPIURL.path! + self.gistID!)!
request = NSMutableURLRequest(URL: updateURL)
request.HTTPMethod = "PATCH"
}
And this is certainly wrong. The better approach would start with:
if let gistID = self.gistID {
// we know gistID is non-nil, and it's already unwrapped...
}
enum ConnectionError: ErrorType {
case Bad(String)
case Worse(String)
case Terrible(String)
}
These classifications for errors are far from sufficient. The severity of the error should be a sub-property of the classification of the error. Our errors should be classified into more specific types. For example:
- No Internet.
- No response from server.
- Invalid credentials.
- Invalid path.
Etc. This is very simplified and probably not a complete list, but hopefully you get the point. Now, these should probably still be in an enum and still be paired with human-readable string descriptions of the problem, and perhaps a severity indication... but we need to start by classifying the type of error.
Why?
Most importantly, because it will determine how someone using your library will respond to the error. This will be a far better indicator of the best course of action for responding to the error than some vague severity that you're classifying yourself (and is probably highly subjective generally).
