cdzombak / thingshub

[UNMAINTAINED] Synchronize issues assigned to you from a Github repo into Things.
MIT License
18 stars 2 forks source link

Clean up authentication manager to expose a signal-based API #1

Closed andrewsardone closed 10 years ago

andrewsardone commented 10 years ago

@cdzombak, this is in response to some concerns we discussed about the “callback-hell” nature of CDZGithubAuthManager.

I think the callback-hell and approachability of the code derives from the fact that the method returns void. The method is an accessor of sorts – we simply want an authenticated OCTClient. Unfortunately, without raising our level of abstraction we have to resort to a callback block, despite our language actually having a notion of return types.

So, starting from the interface, let's restructure the method like so:

/// Returns a `RACSignal` that will send the appropriate `OCTClient` for the
/// given `githubLogin`.
+ (RACSignal *)authenticatedClientForUsername:(NSString *)githubLogin;

Now client code could bind to that change over time. It looks like you create a CDZIssuesSyncEngine from authenticated clients, so you could make that relationship explicit with something like this:

RAC(self, syncEngine) = [[RACSignal authenticatedClientForUsername:@"cdzombak"] map:^id(id client) {
    return [[CDZIssueSyncEngine alloc] initWithAuthenticatedClient:client];
}];

It's a somewhat trivial example. but the goal is to declare these functional relationships as opposed to imperatively writing code within callbacks.

I could walk through some of the changes or you could ping me with questions in-line.

andyfowler commented 10 years ago

+1

cdzombak commented 10 years ago

@andrewsardone thanks for the help. I'll merge in just as soon as I am confident I understand everything here :)