Shopping cart logic for authenticated users and guests

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
{

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

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

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

}


My revision

if([APPDELEGATE isCommerceZone])
{

} 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]];


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.

• all valid points, the task given was to reduce if/else blocks so with your notes taken into consideration is my revision sufficiently executing the same way? – erik Jul 29 '14 at 16:33
• I don't see any good way or reason to reduce the number of if-else blocks. Making each explicit is good and clear for maintenance. The fourth block has both as false conditions and it does nothing. This block probably also gets optimized away by the compiler as a no-op, but it could be commented out just the same. – uchuugaka Aug 4 '14 at 9:06