search

twitter / ios-twitter-network-layer

Twitter Network Layer is a scalable and feature rich network layer built on top of NSURLSession for Apple platforms
Apache License 2.0
573 stars 38 forks source link

Improvements in code style. #5

Closed lolgear closed 6 years ago

lolgear commented 6 years ago

Well, I am a bit confused about your code style. Well, it sometimes old, sometimes mis-conceptual, sometimes doesn't fit objective-c style.

For example:

// TNLSafeOperation
// NULL and nil are equivalent here, doesn't matter, of course.
// but in objective-c self.completionHandler = nil; is more appropriate
//  [super setCompletionBlock:nil];
- (void)tnl_clearCompletionBlock
{
    [super setCompletionBlock:NULL];
}

Moreover, you don't use auto type inference that cause a lot of boring reading. Do not tell me that you build your apps with "not brand new xcode and bleeding-edge sdk". So, bring new features of clang and llvm to your "codebase at the end of universe".

        NSProcessInfo *processInfo = [NSProcessInfo processInfo];

will turn into

        __auto_type processInfo = [NSProcessInfo processInfo];

Again, look at TNLSafeOperation static function in .m file.

For what reason you put static function here? Is it a C API? I do not see any invocation anywhere but in this file.

Oh, similar mistake to a lot of frameworks.

NSString * const TNLBackgroundRequestOperationDidCompleteNotification = @"TNLBackgroundRequestOperationDidCompleteNotification";
NSString * const TNLBackgroundRequestURLRequestKey = @"URLRequest";
NSString * const TNLBackgroundRequestResponseKey = @"response";
NSString * const TNLBackgroundRequestURLSessionConfigurationIdentifierKey = @"identifier";
NSString * const TNLBackgroundRequestURLSessionTaskIdentifierKey = @"taskIdentifier";
NSString * const TNLBackgroundRequestURLSessionSharedContainerIdentifierKey = @"sharedContainerIdentifier";

I don't want to see these keys, well, never. I want to see a group of them like, for example, TNLBackgroundRequest__Keys.

@interface TNLBackgroundRequest__Keys : NSObject
+ (NSString *)OperationDidCompleteNotification;
+ (NSString *)URLRequestKey;
+ (NSString *)ResponseKey;
+ (NSString *)URLSessionConfigurationIdentifierKey;
+ (NSString *)URLSessionTaskIdentifierKey;
+ (NSString *)URLSessionSharedContainerIdentifierKey;
@end

Yes, I add additional interface that mimic string enums ( not really ) in "modern" swift.

Not really sure why you use static everywhere.

static dispatch_queue_t _URLSessionTaskOperationPropertyQueue(void);
static dispatch_queue_t _URLSessionTaskOperationPropertyQueue()
{
    static dispatch_queue_t sQueue;
    static dispatch_once_t onceToken;
    dispatch_once(&onceToken, ^{
        sQueue = dispatch_queue_create("TNLRequestOperation.URLSessionTaskOperation.atomic.queue", DISPATCH_QUEUE_CONCURRENT);
    });
    return sQueue;
}

Perfect place for them is @interface with class methods.

Sometimes you use explicit obj == nil and obj != nil. But when your demons come out, you prefer if (obj) and if (!obj).

No code style at all?

NSProgrammer commented 6 years ago

Thank you for you comments, Dmitry. I appreciate you taking time to volunteer feedback. I’ll work to speak to each of your comments.

NULL for blocks

So, blocks are an interesting case since they are a C construct but in Objective-C are implemented supporting object semantics. For C constructs, NULL is preferable since nil is not present in C. For objects, nil is preferable. Blocks, however, are both. Blocks were not always objects, however, and that legacy is probably showing in my code when I use NULL for a block.

I think this is a fair critique though, and to modernize, I will take this as a task to replace NULLs we have for blocks with nil.

lack of __auto_type

I’m afraid non-boring reading is not a goal for our project. Concrete and explicit code is a goal, unambiguous typing and avoiding performance impact are all goals however. When you use auto_type, that leads to type inference, which has a compile time cost. If you use Swift in a large code base you might be familiar with how impactful this can be on compilation times and is frankly something I would like Swift to address with the ability to opt out of type inference. Additionally, __auto_type fails preserve nullability attributes. If you use auto_type, you can end up passing a nullable variable as a nonnull without any compiler complaint, which is an unacceptable risk.

I think it is great that compilers are advancing to add syntactic sugar support, but this is a case where we are intentional in not supporting a specific compiler feature. Consuming code is obviously free to do so, but the code we encapsulate in our open source projects will remain safe and explicit while avoiding imposing an unnecessary compile time cost on those building our project.

constants as extern C const values

I can see that you have a preference for the syntax you like to type when accessing constants in objective-c, which is absolutely fine to have as preference. It is not an absolute however that your suggested way is the correct way, and us having our constants defined as we have is hardly a mistake.

For one, it is the same style choice that Apple chooses for their frameworks and thus more consistent with Objective-C codebases at large. Notably, there is also a cost incurred with every Objective-C selector you define that manifests in both binary size and dynamic library load times. There is cost to extern C constants too, but not as high as the Objective-C methods. @ricomariani, whom I respect in the world of software development, would likely point out that we are not doing enough to improve fix up costs in library loads from extern’d constants and Objective-C selectors, but we do work to not make things worse with patterns that would incur cost with no real benefit other than syntactic sugar. As an open source project, we aim to not impose undue cost on our consumers.

static functions

This is actually the same as for why we don’t put constants in methods, but even moreso. For internal code that we do not expose, we aim to not incur undue binary size cost or dynamic framework load time cost and so static functions are used a great deal. They also have the benefit of being inlined by the compiler if they are only referenced in one place offering modular code without method/function call overhead. I can understand this being potentially dry code (or even unpleasant for some) but for encapsulated code, this is an important choice we make to the benefit of all our consumers. We will likely get even more aggressive moving private code to static C functions too.

obj == nil, obj != nil, obj, !obj

We have many developers in the code base and many reviewers and you are correct that we don’t have an explicit guide for nil checking so it can waffle. However, if we were to choose, we would ensure defensive programming practices were in place and forbid “== nil” checks as it is too close to “= nil” and could lead to accidental bugs, so I would opt for “nil == obj” or “!obj”. I will take this feedback to clean up “obj == nil” in the project.

Another place where enforcement is important is when a boolean argument or return value is based on a reference not being NULL/nil. It can be unsafe to treat a reference as “true”, so coersing the reference to true or false is necessary to avoid potential bugs such as (0x1000 == true) which would evaluate to false though is likely not the intent. We do enforce this as a style, so please note places where we fail to uphold that standard.

If you don’t feel like our choices meet your needs or desires, feel free to fork TNL and make those changes for your use cases. We have the most permissible license with apache 2.0, so you are free to do whatever you like with the code we are sharing with the open source community.

Again, thank you for the feedback on Twitter Network Layer. Feel free to provide more feedback, it is always welcome.

If I might provide a suggestion, when providing feedback to open source projects, it is helpful to keep in mind they exist in a community of volunteers who only want to help the community with their contributions. It can be better for facilitating discussion to present critiques as inquiries with genuine interest in learning such that if there is a problem, it can be surface in a supportive and collaborative fashion. Being confrontational can prevent collaboration and can hurt the value you are providing with feedback and insight that wasn’t considered before you brought it up. Keep the feedback coming, it’s welcome and valuable — just keep in mind a friendly tone in this volunteer community can benefit everyone involved.

NSProgrammer commented 6 years ago

reopening until action items are merged

lolgear commented 6 years ago

Let's speak about code. More issues.

Categories of NS-classes.

It is an external library. While it is not providing custom features to these base/core classes like high-order functions or any DSL, you don't need to extend these classes at all. Moreover, some functions exist in an application that will use your library and at the end developer will do:

// NSData+SomeMethod
- (void)someMethod {
   // equal to TNL counterpart
   [NSData tnl_someMethod];
}

Is it a bless or a curse? Of course, it is a curse. You don't have any rights to extend base classes. It is the same as you provide additional framework inside your framework that injects core framework.

Correct extension of core framework in your case is:

@interface TNLNSDataExtension
+ (void)someMethod data:(NSData *)data;
+ (void)someMethodFor:(NSData *)data;
@end

Auto type inference.

I hardly believe that it even changes anything. I have codebase of an app which is around ~150k LoC. MacBook Pro 15 '' ( Late 2011 ) archives this app in 160-200 seconds or less ( 8 frameworks, 32/64 arch support, assets ). It has 3365 tasks at archive action with 3141 occurrences of __auto_type.

Yes, if you compile iOS on MacBook Pro, it can change something. But wait. You don't.

Also you said about nullability. I insist of using __auto_type in places where it should fit well. Moreover, if you said about nullability, show me places where __auto_type will not do it job.

Consider:

- (nullable NSSet *)tnl_keysMatchingCaseInsensitiveKey:(NSString *)key
{
    NSMutableSet *keys = nil;
    TNLAssert([key isKindOfClass:[NSString class]]);
    if ([key isKindOfClass:[NSString class]]) {
        for (NSString *otherKey in self.allKeys) {
            // logic error here.
            TNLAssert([key isKindOfClass:[NSString class]]);
            if ([otherKey caseInsensitiveCompare:key] == NSOrderedSame) { // TWITTER_STYLE_CASE_INSENSITIVE_COMPARE_NIL_PRECHECKED
                if (!keys) {
                    keys = [NSMutableSet set];
                }
                [keys addObject:otherKey];
            }
        }
    }
    return keys;
}
- (nullable NSArray *)tnl_objectsForCaseInsensitiveKey:(NSString *)key
{
    TNLAssert(key);
    NSSet *keys = [self tnl_keysMatchingCaseInsensitiveKey:key];
    NSMutableArray *objects = (keys.count > 0) ? [NSMutableArray array] : nil;
    for (NSString *otherKey in keys) {
        [objects addObject:self[otherKey]];
    }
    return objects;
}

and auto_typed variant with generic improvements.

- (nullable NSSet <NSString *>*)tnl_keysMatchingCaseInsensitiveKey_VariantB:(NSString *)key
{
    TNLAssert([key isKindOfClass:[NSString class]]);

    if (![key isKindOfClass:[NSString class]]) {
        return nil;
    }

    __auto_type allKeys = (NSArray <NSString *>*)self.allKeys;
    __auto_type result = (NSMutableSet <NSString *>*)[NSMutableSet new];

    [allKeys enumerateObjectsUsingBlock:^(NSString * _Nonnull obj, NSUInteger idx, BOOL * _Nonnull stop) {
        TNLAssert([obj isKindOfClass:[NSString class]]);
        if ([obj caseInsensitiveCompare:key] == NSOrderedSame) {
            [result addObject:obj];
        }
    }];
    return result.allObjects.count > 0 ? result : nil;
}

- (nullable NSArray *)tnl_objectsForCaseInsensitiveKey_VariantB:(NSString *)key
{
    TNLAssert(key);
    __auto_type keys = [self tnl_keysMatchingCaseInsensitiveKey:key];
    // you still check for nil here. ok.
    __auto_type objects = (keys.count > 0) ? [NSMutableArray array] : nil;
    [keys enumerateObjectsUsingBlock:^(NSString * _Nonnull obj, BOOL * _Nonnull stop) {
        [objects addObject:self[obj]];
    }];
    return objects;
}

I rewrite your example in auto_type with generics. It is more readable and has fewer errors. Check your code. It has minimum one logic error or I even don't understand your code.

Interesting observation. Check out these functions:

+ (nullable instancetype)operationSafetyGuard
{
    static TNLOperationSafetyGuard *sGuard = nil;
    static dispatch_once_t onceToken;
    dispatch_once(&onceToken, ^{
        NSOperatingSystemVersion version = { 7, 0, 0 }; /* arbitrarily default as iOS 7 (minimum version for TNL) */
// ....
}

and

+ (BOOL)tnl_URLSessionCanReceiveResponseViaDelegate
{
    static BOOL sBugExists;
    static dispatch_once_t onceToken;
    dispatch_once(&onceToken, ^{

        NSProcessInfo *processInfo = [NSProcessInfo processInfo];
        const NSOperatingSystemVersion OSVersion = [processInfo respondsToSelector:@selector(operatingSystemVersion)] ? processInfo.operatingSystemVersion : (NSOperatingSystemVersion){ 0, 0, 0 };
// ...
}

Did you see that you use implicit conversion to structure in first function? This kind of style is impossible with __auto_type.

Also, compare this code:

NSArray *TNLProtocolClassesForProtocolOptions(TNLRequestProtocolOptions options)
{
    if (!options) {
        return nil;
    }

    NSMutableArray *protocols = [[NSMutableArray alloc] init];
    if (TNL_BITMASK_HAS_SUBSET_FLAGS(options, TNLRequestProtocolOptionPseudo)) {
        [protocols addObject:[TNLPseudoURLProtocol class]];
    }

    return protocols;
}

TNLRequestProtocolOptions TNLProtocolOptionsForProtocolClasses(NSArray * __nullable protocols)
{
    TNLRequestProtocolOptions options = 0;
    for (Class c in protocols) {
        if ([c isSubclassOfClass:[TNLPseudoURLProtocol class]]) {
            options |= TNLRequestProtocolOptionPseudo;
        }
    }

    return options;
}

to

NSArray <Class>*TNLProtocolClassesForProtocolOptions_VersionB(TNLRequestProtocolOptions options)
{
    if (!options) {
        return nil;
    }

    __auto_type protocols = [[NSMutableArray alloc] init];
    if (TNL_BITMASK_HAS_SUBSET_FLAGS(options, TNLRequestProtocolOptionPseudo)) {
        [protocols addObject:[TNLPseudoURLProtocol class]];
    }

    return protocols;
}

TNLRequestProtocolOptions TNLProtocolOptionsForProtocolClasses_VersionB(NSArray <Class>* __nullable protocols)
{
    TNLRequestProtocolOptions options = 0;
    // enumerateObjectsUsingBlock automatically insert type, so, I prefer it here instead of fast enumeration.
    [protocols enumerateObjectsUsingBlock:^(Class  _Nonnull obj, NSUInteger idx, BOOL * _Nonnull stop) {
        if ([obj isSubclassOfClass:[TNLPseudoURLProtocol class]]) {
            options |= TNLRequestProtocolOptionPseudo;
        }
    }];
    return options;
}

Not the best place to compare, but it is now more readable for a big amount of developers.

Too much runtime?

Well, I hardly believe that objc_setAssociatedObject and objc_getAssociatedObject are required in good designed software.

It is a cheat that adds invisible tissue between two objects. It is so implicit.

Outdated code.

Your deployment target is iOS 8. However, somewhere in project you still check target version ( >= 8 ). But, it can be nice if not an one thing - you sometimes check methods that SURELY exists.

// WHAT?
// Maybe I don't know something?
+ (BOOL)tnl_supportsSharedContainerIdentifier
{
    static BOOL sSupported;
    static dispatch_once_t onceToken;
    dispatch_once(&onceToken, ^{
        // Get the default config and check if it responds
        NSURLSessionConfiguration *defaultConfig = [NSURLSessionConfiguration defaultSessionConfiguration];
        sSupported = [defaultConfig respondsToSelector:@selector(setSharedContainerIdentifier:)];
    });
    return sSupported;
}

Well, I inspected project. Yes, a lack of management. Nice.

Deployment targets:

Project:

Targets:

Moreover, you have a lot of #if TARGET_ stuff, which is also outdated in some cases. Again, nearly everyone ( and also you ), will use this library with latest Xcode ( latest Xcode also doesn't support old OS, thus, no fears to use latest clang/llvm and other features ).

Consider:

+ (BOOL)tnl_URLSessionSupportsDecodingBrotliContentEncoding
{
    static BOOL sBrotliSupported = NO;
    static dispatch_once_t onceToken;
    dispatch_once(&onceToken, ^{

        NSProcessInfo *processInfo = [NSProcessInfo processInfo];
        if (![processInfo respondsToSelector:@selector(operatingSystemVersion)]) {
            // version is too low
            return;
        }

        const NSOperatingSystemVersion OSVersion = processInfo.operatingSystemVersion;
        (void)OSVersion;
#if TARGET_OS_IPHONE
    #if defined(__IPHONE_11_0) && (__IPHONE_OS_VERSION_MAX_ALLOWED >= __IPHONE_11_0)
        if (OSVersion.majorVersion >= 11) {
            sBrotliSupported = YES;
        }
    #endif
#elif TARGET_OS_TV
    #if defined(__TVOS_11_0) && (__TV_OS_VERSION_MAX_ALLOWED >= __TVOS_11_0)
        if (OSVersion.majorVersion >= 11) {
            sBrotliSupported = YES;
        }
    #endif
#elif TARGET_OS_WATCH
    #if defined(__WATCHOS_4_0) && (__WATCH_OS_VERSION_MAX_ALLOWED >= __WATCHOS_4_0)
        if (OSVersion.majorVersion >= 4) {
            sBrotliSupported = YES;
        }
    #endif
#elif TARGET_OS_OSX
    #if defined(__MAC_10_13) && (__MAC_OS_X_VERSION_MAX_ALLOWED >= __MAC_10_13)
        if (OSVersion.majorVersion > 10) {
            // Assume post "10" will have brotli
            sBrotliSupported = YES;
        } else if (OSVersion.majorVersion == 10 && OSVersion.minorVersion >= 13) {
            sBrotliSupported = YES;
        }
    #endif
#else
        // Unexpected target, assume it cannot be used
        sBrotliSupported = NO;
#endif

    });

    return sBrotliSupported;
}

@end

and 

+ (BOOL)tnl_URLSessionSupportsDecodingBrotliContentEncoding_VersionB
{
    static __auto_type sBrotliSupported = NO;
    static dispatch_once_t onceToken;
    dispatch_once(&onceToken, ^{
        __auto_type processInfo = [NSProcessInfo processInfo];
        const __auto_type OSVersion = processInfo.operatingSystemVersion;
        if (@available(macOS 10.13.0, iOS 11.0.0, tvOS 11.0.0, watchOS 4.0.0)) {
            sBrotliSupported = YES;
        } else {
            sBrotliSupported = NO;
        }
    });
    return sBrotliSupported;
}

Static functions and extern constants.

Let me search through project again.

string results in files
respondsToSelector 109 18

Is it enough to optimize?

Plan

So, after all there is a plan to remove outdated code.

Belief.

I sincere believe that this project has a lot of abilities and complex parts which are empowered by ObjC dynamic nature.

However, it wasn't polished well and it has hidden errors.

UPD Optional style for generic types:

__auto_type result = [NSMutableSet<NSString *> new];
NSProgrammer commented 6 years ago

Thank you again, I've responded to each of your comments below.

Categories of NS-classes.

We have categories on Apple classes to extend abilities and use a prefix of tnl_ to ensure there will be no conflict between TNL code, Apple code or any consuming code. Consumers can use indirection to access TNL extensions, or access them directly, or just ignore their existence -- no problem. Apple calls out that code that is not Apple code and is extending Apple code (categories or subclassing large classes) needs 3 character (or more) prefixing to avoid conflict. We adopt this rigorously for the safety of consumers.

We needn't debate this, I think we are at a difference of opinion here. We will continue with our rigor using prefixed method names for categories.

Auto type inference.

We have 1.5 million lines of code, and type inference absolutely has impact (MBP 15", late 2014). If you find it negligible, that is great, however it is hardly worth us risking impacting users for little gain in encapsulated code.

There's plenty of commentary of nullability being lost with __auto_type including radars that Apple is not going to address: https://openradar.appspot.com/27062504

That alone make __auto_type unviable for a rigorous implementation.

Your example for tnl_keysMatchingCaseInsensitiveKey: you say there is a logic error, which I see now that we are asserting the wrong variable, however that is a typo that could easily exist in either implementation -- I'll fix that though. The rewrite you provide basically has minimal style differences with the use of __auto_type, but otherwise is no different. I'd be fine with your rewrite without the __auto_type being used. You say you don't understand the code that you rewrote, and I believe that, but there's hardly enough change between the implementations for us to revise. It seems you care more about avoiding fast enumeration than anything in your examples, which is your preference.

With the NSOperatingSystemVersion conversions, I don't see the problem. C works like this...I should have cast to the specific struct in the first method you called out, but otherwise, I see no issues.

Your example with TNLProtocols appears to just be a preference for __autotype vs explicit type being present, which is a fine preference, but not really compelling for needing a change.

I think you might be seeing more value in __auto_type than I can see, and perhaps even conflating what it does with something that it doesn't do. That's fine, however let's agree to disagree here as we won't be incurring the risk of nullability being lost nor do we want to stop having explicit types present for readability.

Too much runtime?

I agree, associated objects would hardly be necessary in good software design, however the Apple frameworks fail to offer the context necessary to accomplish associations needed in our robust network layer and so we are working around those deficiencies in the framework we were provided and use associated objects to do so. We only use them as a last resort and, in our limited use, there is no other reasonable option. These associated objects are always invisible to the consumer of TNL, they never need to know about it and so the encapsulated implicitness you cite is not an exposed problem.

Outdated code.

You have a good point here. We had support for a static library for TNL that was iOS 7+. Prior to open source, we did eliminate that target, and so there is not iOS 7 need anymore in TNL. I am going to take this as an action item for a deep revisiting of our code to 1) presume iOS 8+ support and 2) move to @available checks.

Regarding the brotli check, however, that code is necessary. If the target does not link to iOS 11, Brotli will not be used, thus it is insufficient to just check the runtime OS version, we need to check the compiled target version too. I will add a comment to make this explicitly clear.

ISSUE #6

Static functions and extern constants.

You just searched for respondsToSelector, which is inaccurate. The vast majority of the respondsToSelector checks are for accessing optional methods/properties of TNL defined protocols. In reality, there are about 10 cases where we use respondsToSelector to check that the Apple framework API is available. Those will get cleaned up with ISSUE #6 and what will remain is going to be just TNL protocol selector checks which are completely valid.

Also, I don't really understand how you are relating respondsToSelector to static functions and extern constants and asking "enough to optimize?"

Plan

Closing

Going forward, we would like to address issues one at a time. Please file individual issues per issue rather than a single issue covering multiple things.

NSProgrammer commented 6 years ago

Small update, I went through the code and there was only 1 place we set NULL for a block. I have updated that on the twitter repo and it will be merge to this github repo on the next integration.