Skip to content
This repository was archived by the owner on Nov 14, 2023. It is now read-only.

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

Merged
merged 2 commits into from
Jan 18, 2014

Conversation

andrewsardone
Copy link
Collaborator

@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
Copy link

+1

completionBlock(nil, error);
}];

return [OCTClient signInAsUser:user password:password oneTimePassword:twoFactorCode scopes:CDZThingsHubGithubScopes];
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Okay, I understand this one :)

@cdzombak
Copy link
Owner

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

@ghost ghost assigned cdzombak Jan 16, 2014
cdzombak added a commit that referenced this pull request Jan 18, 2014
Clean up authentication manager to expose a signal-based API
@cdzombak cdzombak merged commit 1c0579d into cdzombak:master Jan 18, 2014
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants