Shopping cart logic for authenticated users and guests

Question

I am tasked with cleaning up someone else's Objective-C code, and will admit it is not my language of choice. I am not sure if this is a valid question but was hoping someone could double check my attempt to clean up a method written by someone else, and make sure I didn't miss some minor detail or over simplify the code.

The original method written by someone else

if([[UserManager sharedManager]isUserLoggedIn] && ([APPDELEGATE isCommerceZone]))// Auth and commerce
    {
        [self showPopUpAddToCartOrExpressOrder:NO];

        UIAccessibilityPostNotification(UIAccessibilityScreenChangedNotification, _addToCartOrExpressOrderView);

    }
    else if([[UserManager sharedManager]isUserLoggedIn] && (![APPDELEGATE isCommerceZone])) // Auth and noncommerce
    {
          [self onModifyItemDone:nil];

    }
    else if (![[UserManager sharedManager]isUserLoggedIn] && ([APPDELEGATE isCommerceZone]))// guest and commerce
    {
        [self showPopUpAddToCartOrExpressOrder:YES];
       UIAccessibilityPostNotification(UIAccessibilityScreenChangedNotification, _addToCartOrExpressOrderView);

    }
    else if (![[UserManager sharedManager]isUserLoggedIn] && (![APPDELEGATE isCommerceZone]))// guest and noncommerce
    {
        /// Do nothing

    }

My revision

if([APPDELEGATE isCommerceZone])
    {

        [self showPopUpAddToCartOrExpressOrder:![[UserManager sharedManager]isUserLoggedIn]];

        UIAccessibilityPostNotification(UIAccessibilityScreenChangedNotification,_addToCartOrExpressOrderView);


    } else if([[UserManager sharedManager]isUserLoggedIn]) // Auth and noncommerce
    {
        [self onModifyItemDone:nil];
    }

In Objective-C, is this the equivalent to saying this boolean equals the opposite of this other boolean?

[self showPopUpAddToCartOrExpressOrder:![[UserManager sharedManager]isUserLoggedIn]];

Answer 1

There are two primary concerns I have.

---

First, rather than accessing [[UserManager sharedManager]isUserLoggedIn] twice, let's access it just once:

BOOL loggedIn = [[UserManager sharedManager] isUserLoggedIn];

Now just use the loggedIn variable in place of calling these methods.

In this case, it's not going to make a huge difference, but it's a good habit to get into. If you're going to use the result of a method call multiple times within a method, call the original method once and save the result to a local variable. This can make the code more clear (with appropriately named local variables) and will be more efficient with more complex code.

---

Second, APPDELEGATE, I'm going to assume, is a #define-ed macro. You should eliminate uses of this macro. For one, you should just generally be avoiding #define-ing stuff, and for two, this runs into the same trouble as calling that isUserLoggedIn method repeatedly, but masks it behind APPDELEGATE.

Given that you're cleaning up someone else's code, I recommend just eliminating uses of the macro, but don't actually delete the macro definition until you've cleaned them all up eventually.

Instead, use a function that returns the app delegate reference. Call that function once and store a local reference to it for future uses within the method.

A function would look very similar to the macro:

MyAppDelegate * MyAppDelegate() {
    return [[UIApplication sharedApplication] delegate];
}

Now...

MyAppDelegate *appDelegate = MyAppDelegate();

And use appDelegate as your reference to app delegate.