Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Some parts of the code look strange ==> refactoring might be needed #3

Open
dodikk opened this issue Jul 26, 2018 · 4 comments
Open

Comments

@dodikk
Copy link

dodikk commented Jul 26, 2018

1). Obsolete AND with true with no reason or value
https://github.com/jivesoftware/JiveAuthenticatingHTTPProtocol/blob/master/Source/JiveAuthenticatingHTTPProtocol/JAHPAuthenticatingHTTPProtocol.m#L256

shouldAccept = YES && [scheme isEqual:@"http"];

This is equivalent to shouldAccept = [scheme isEqual:@"http"]; according to the boolean algebra theory.

2). _cmd can be used for most delegate method forwarding.
https://github.com/jivesoftware/JiveAuthenticatingHTTPProtocol/blob/master/Source/JiveAuthenticatingHTTPProtocol/JAHPAuthenticatingHTTPProtocol.m#L527

3). "Pure C" assert() function used in objective-c methods instead of NSAssert() or NSParameterAssert() that provide better error messages and crash dumps.

4). Retain cycles and possibly memory leaks. No single "weakify" statement across the entire project.
For example :

    [self performOnThread:nil
                    modes:nil
                    block:^void()
    {
        // `self` captured by the block 
        //  `self` cannot be destroyed while the block "lives" (in the queue, for instance)
        //  both might live forever if not dismissed properly
        //
        [self mainThreadDidReceiveAuthenticationChallenge:challenge
                                        completionHandler:completionHandler];
    }];

If such lifetime behaviour is desired, some memory management related comments are needed.

@hborders
Copy link
Collaborator

I really appreciate your feedback, and I'm glad someone else found this useful, but I don't believe anyone at Jive is maintaining this anymore, so your best bet is to fork this project and update it yourself.

Obsolete AND with true with no reason or value
"Pure C" assert() function used in objective-c methods instead of NSAssert() or NSParameterAssert() that provide better error messages and crash dumps.

As I explain in the README, most of this came directly from Apple's CustomHTTPProtocol, and I made no effort to clean it up. I just namespaced the classes so it wouldn't conflict with other libraries that used the example code. Obviously Apple had some sub-optimal code that I didn't improve.

_cmd can be used for most delegate method forwarding.

I'm not sure what you mean. _cmd here would be @selector(mainThreadDidReceiveAuthenticationChallenge:completionHandler:), which is obviously not the proper delegate method.

Retain cycles and possibly memory leaks.

This isn't a retain cycle. -performOnThread:modes:block: releases the block, so the cycle is properly broken. Weakifying isn't necessary in these one-off cases where a block is going to fire quickly and won't be cancelled.

@dodikk
Copy link
Author

dodikk commented Jul 27, 2018

I'm not sure what you mean. _cmd here would be ... which is obviously not the proper delegate method.

My bad. I've posted a wrong link. The _cmd could have been applied in the @implementation JAHPQNSURLSessionDemux. In that class the selectors do match the enclosing function.

dodikk pushed a commit to dodikk/JiveAuthenticatingHTTPProtocol that referenced this issue Jul 27, 2018
dodikk pushed a commit to dodikk/JiveAuthenticatingHTTPProtocol that referenced this issue Jul 27, 2018
* assert() ==> NSParameterAssert()
* using _cmd instead of selector literals where appropriate
* JAHPQNSURLSessionDemuxTaskInfo extracted to a separate file
* a few typedefs introduced for convenience
* a few local variables extracted
dodikk pushed a commit to dodikk/JiveAuthenticatingHTTPProtocol that referenced this issue Jul 27, 2018
* Extracted some invariants as local variables
* Removed some duplicated calls
dodikk pushed a commit to dodikk/JiveAuthenticatingHTTPProtocol that referenced this issue Jul 27, 2018
@dodikk
Copy link
Author

dodikk commented Jul 27, 2018

Some refactoring done in #7

@hborders
Copy link
Collaborator

I remember now why I didn't do any reformatting or refactoring of the sample code. I wanted to be sure it would be easy to diff the example code if it ever changed, and I wanted to update it. You could add a line in the README explaining that strategy. Refactoring the example code is fine, but please keep the existing formatting.

dodikk pushed a commit to dodikk/JiveAuthenticatingHTTPProtocol that referenced this issue Jul 27, 2018
dodikk pushed a commit to dodikk/JiveAuthenticatingHTTPProtocol that referenced this issue Jul 27, 2018
dodikk pushed a commit to dodikk/JiveAuthenticatingHTTPProtocol that referenced this issue Jul 27, 2018
dodikk pushed a commit to dodikk/JiveAuthenticatingHTTPProtocol that referenced this issue Jul 27, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

2 participants