6
\$\begingroup\$

I'm developing an iOS application where data is fetched from a third-party REST API. Each request must contain an authorization token.

In order to implement this, I've written a fetchToken withCompletionBlock:(void (^)(ASToken*) token,NSError * error) method in AppDelegate:

-(void) fetchTokenWithCompletionBlock:(void (^)(ASToken * token, NSError * error)) completionBlock
{
    if (!_token) {
        _token = [self retrieveTokenFromKeychain];
    }

    if (!_token || _token.isExpired) {
        [ASMyApi loadTokenWithcompletionBlock:^(ASToken *token, NSError*  error) {

            if (token) {
                _token = token;
                [self saveTokenToKeychain: token];
            }else{
                completionBlock(nil,&error);
            }

        }];
    }else{
        completionBlock(_token,nil);
    }
}

Before every requests that requires Authorization, fetchToken withCompletionBlock:(void (^)(ASToken*) token,NSError ** error) is called to retrieve the token. Here's an example:

-(IBAction) loadCategories
{
    [self.refreshControl beginRefreshing];
    [((AppDelegate *)[UIApplication sharedApplication].delegate) fetchTokenWithCompletionBlock:^(ASToken *token, NSError *error) {

        if (token) {
            [ASMyApi loadCategories:token.token completionBlock:^(NSArray *categories, NSError *error) {

                [self.refreshControl endRefreshing];

                if (categories){

                    self.categories = categories;

                }else{

                    [self monitorNetworkReachability];

                }

                [self updateUI];

            }];

        }else{

            [self.refreshControl endRefreshing];
            [self monitorNetworkReachability];
        }

    }];
}

I'm not happy with this code for the following reasons:

  • The nested nature of the callbacks makes it look ugly and somewhat unreadable

  • Too much repetition: The code to get the token is verbose and must be called before each request. The failure code is also repeated.

What I do like about this approach is that it ensures that there's always a valid token before sending the request (as opposed to having the request fail, and then requesting a new token).

I'd appreciate a second-opinion on this design and input on how it might be refactored.

\$\endgroup\$
  • 1
    \$\begingroup\$ I'm trying to figure out why the outer completion block needs to be an NSError ** exactly. This seems confusing to me. We usually reserve NSError ** for when we're passing it into a method. I don't know if I've ever seen it as a completion block argument. \$\endgroup\$ – nhgrif Nov 22 '15 at 15:33
  • \$\begingroup\$ My reasoning was that if fetchTokenWithCompletionBlock: fails, the NSError would be passed to the completionBlock which is in the ViewController. and the ViewController would then handle the error (e.g. by displaying an alert or something). I'm quite new to iOS so it might not actually be a good idea. \$\endgroup\$ – W.K.S Nov 22 '15 at 17:29
  • \$\begingroup\$ Yeah, but why doesn't NSError * work? Why do you need the pointer-to-pointer? \$\endgroup\$ – nhgrif Nov 22 '15 at 17:30
  • \$\begingroup\$ I'm looking at it and even I don't know why I did that! Sorry, that's my bad. \$\endgroup\$ – W.K.S Nov 22 '15 at 17:35
  • \$\begingroup\$ It seems that you only check the token in local side. However the token may be invalid in the server side for unknown reasons. So it would be better to handle token error message from server side and auto refresh token after that. \$\endgroup\$ – xi.lin Feb 1 '16 at 8:04
6
+50
\$\begingroup\$

You could introduce a delegate instead passing another block in loadCategories method.

First, the protocol:

@protocol ASMyApiCategoriesDelegate <NSObject>

    @required
    - (void) ASMyApiCategoriesDidLoad:(NSArray *)categories;
    - (void) ASMyApiCategoriesDidFailToLoadWithError:(NSError *)error

@end

You change the signature of + [ASMyApi loadCategories:completionBlock:] to accept a delegate:

+ (void) loadCategoriesToken:(id)token delegate:(id <ASMyApiCategoriesDelegate>)delegate
{
    NSArray *categories;
    // code that loads categories

    if (categories)
    {
        [delegate ASMyApiCategoriesDidLoad:categories];
    }
    else
    {
        [delegate ASMyApiCategoriesDidFailToLoadWithError:nil];
    }
}

In your view controller you'll have to implement the required methods from ASMyApiCategoriesDelegate protocol and update loadCategories method.

- (void) ASMyApiCategoriesDelegateDidLoad:(NSArray *)categories
{
    [self.refreshControl endRefreshing];
    self.categories = categories;
    [self updateUI];
}

- (void) ASMyApiCategoriesDelegateDidFailToLoadWithError:(NSError *)error
{
    [self.refreshControl endRefreshing];
    [self monitorNetworkReachability];
}

- (IBAction) loadCategories
{
    [self.refreshControl beginRefreshing];
    [((AppDelegate *)[UIApplication sharedApplication].delegate) fetchTokenWithCompletionBlock:^(ASToken *token, NSError *error) {
        if (token)
        {
            [ASMyApi loadCategories:token.token delegate:self];
        }
        else
        {
            [self ASMyApiCategoriesDidFailToLoadWithError:nil];
        }
    }];
}
\$\endgroup\$
  • \$\begingroup\$ That looks like a good way of doing it. The API loads other kind of objects besides categories e.g. products, deals e.t.c. In this case - would make sense to have one delegate with response data of type id or to declare a delegate for each kind of response object? \$\endgroup\$ – W.K.S Nov 25 '15 at 11:08
  • \$\begingroup\$ Categories, products, deals: they look like all are arrays. In this case I would use the same response for all. \$\endgroup\$ – eli Nov 25 '15 at 14:18

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service, privacy policy and cookie policy

Not the answer you're looking for? Browse other questions tagged or ask your own question.