Get acceptableStatusCodes via instance method #319

Closed
wants to merge 1 commit into
from

Conversation

Projects
None yet
2 participants
@tewha
Contributor

tewha commented Apr 25, 2012

Instead of calling the class method acceptableStatusCodes directly, this calls an instance method acceptableStatusCodes that calls the class method.

This:

  • improves readability
  • replaces a redundant call to acceptableStatusCodes
  • allows developers to subclass AFHTTPRequestOperation and make the subclass support per-instance acceptableStatusCodes
@tewha

This comment has been minimized.

Show comment Hide comment
@tewha

tewha Apr 25, 2012

Contributor

I didn't realize a pull request created an issue, so here's the information that was in the previous issue:

The context here is I am making calls to a REST API. Some of the APIs I access indicate success in different ways. For instance, one of the APIs I connect to defines 403 as "already signed in." This shouldn't be treated as a fatal error when trying to sign in. But the way errors are handled varies between different APIs.

Contributor

tewha commented Apr 25, 2012

I didn't realize a pull request created an issue, so here's the information that was in the previous issue:

The context here is I am making calls to a REST API. Some of the APIs I access indicate success in different ways. For instance, one of the APIs I connect to defines 403 as "already signed in." This shouldn't be treated as a fatal error when trying to sign in. But the way errors are handled varies between different APIs.

@mattt

This comment has been minimized.

Show comment Hide comment
@mattt

mattt May 1, 2012

Contributor

I'm not sure that the example you're citing is a particularly good use case. Any way you cut it, 403 is an error code. You asked to sign in, and that either succeeds in signing you in or fails, whether it's because you mistyped your password, or you were already signed in. Either way, you're not getting the resource you expected, so it doesn't really make sense to call that a success. The point is that it's up to you to handle error codes correctly—errors are only what you make of them.

Contributor

mattt commented May 1, 2012

I'm not sure that the example you're citing is a particularly good use case. Any way you cut it, 403 is an error code. You asked to sign in, and that either succeeds in signing you in or fails, whether it's because you mistyped your password, or you were already signed in. Either way, you're not getting the resource you expected, so it doesn't really make sense to call that a success. The point is that it's up to you to handle error codes correctly—errors are only what you make of them.

@tewha

This comment has been minimized.

Show comment Hide comment
@tewha

tewha May 1, 2012

Contributor

So you're suggesting instead of writing this:

void(^success)(AFHTTPRequestOperation *operation, id responseObject) = ^(AFHTTPRequestOperation *operation, id responseObject){
    // continue with next operation
};
void(^failure)(AFHTTPRequestOperation *operation, NSError *error) = ^(AFHTTPRequestOperation *operation, NSError *error) {
    // report failure
}

I write this:

dispatch_block_t actual_success = ^{
    // continue with next operation
}
dispatch_block_t actual_fail = ^{
    // report failure
}
void(^success)(AFHTTPRequestOperation *operation, id responseObject) = ^(AFHTTPRequestOperation *operation, id responseObject){
    actual_success();
};
void(^failure)(AFHTTPRequestOperation *operation, NSError *error) = ^(AFHTTPRequestOperation *operation, NSError *error) {
    if ( [error.domain isEqual: AFNetworkingErrorDomain] && ( error.code == NSURLErrorBadServerResponse ) && (operation.response.statusCode == 403 )) {
        actual_success();
    } else {
        actual_fail();
    }

I can live with this, I suppose, but this seems like a lot of extra code (luckily, in only a few rare cases) to cover exactly what acceptableStatusCodes was intended for. What, if not this?

Contributor

tewha commented May 1, 2012

So you're suggesting instead of writing this:

void(^success)(AFHTTPRequestOperation *operation, id responseObject) = ^(AFHTTPRequestOperation *operation, id responseObject){
    // continue with next operation
};
void(^failure)(AFHTTPRequestOperation *operation, NSError *error) = ^(AFHTTPRequestOperation *operation, NSError *error) {
    // report failure
}

I write this:

dispatch_block_t actual_success = ^{
    // continue with next operation
}
dispatch_block_t actual_fail = ^{
    // report failure
}
void(^success)(AFHTTPRequestOperation *operation, id responseObject) = ^(AFHTTPRequestOperation *operation, id responseObject){
    actual_success();
};
void(^failure)(AFHTTPRequestOperation *operation, NSError *error) = ^(AFHTTPRequestOperation *operation, NSError *error) {
    if ( [error.domain isEqual: AFNetworkingErrorDomain] && ( error.code == NSURLErrorBadServerResponse ) && (operation.response.statusCode == 403 )) {
        actual_success();
    } else {
        actual_fail();
    }

I can live with this, I suppose, but this seems like a lot of extra code (luckily, in only a few rare cases) to cover exactly what acceptableStatusCodes was intended for. What, if not this?

@mattt

This comment has been minimized.

Show comment Hide comment
@mattt

mattt May 1, 2012

Contributor

FWIW, actual_success and actual_fail look like they should be instance methods in the controller. Also, you could probably do without the first two checks for the failure block.

Wouldn't you have to add a comparable amount of code to cover a 403 response from the server in success, so you don't try to use non-existent credentials?

Actually, you know what? You could skip the -setCompletionBlockWithSuccess:failure: altogether. If you're looking at an edge case like this where success and failure don't mesh with your reality, you can always set completionBlock yourself, and have it work all the same.

Contributor

mattt commented May 1, 2012

FWIW, actual_success and actual_fail look like they should be instance methods in the controller. Also, you could probably do without the first two checks for the failure block.

Wouldn't you have to add a comparable amount of code to cover a 403 response from the server in success, so you don't try to use non-existent credentials?

Actually, you know what? You could skip the -setCompletionBlockWithSuccess:failure: altogether. If you're looking at an edge case like this where success and failure don't mesh with your reality, you can always set completionBlock yourself, and have it work all the same.

@tewha

This comment has been minimized.

Show comment Hide comment
@tewha

tewha May 1, 2012

Contributor

That's a good alternative. Thanks!

Contributor

tewha commented May 1, 2012

That's a good alternative. Thanks!

@tewha tewha closed this May 1, 2012

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment