Fetching Background Data in iOS View Controller

I am trying to come up with a way to fetch the same data both when the application is running and when it is in the background. The code I have written so far works... I just think it is very messy and could probably be refactored, but I'm not sure where to start.

I have put all the files up on GitHub as they are rather large GitHub Repo.

Here's the code to review though:

```
#import "TabBarViewController.h"
#import "UIView+Border.h"
#import "UIColor+HexColors.h"
#import "Achievement.h"
#import "Achievement+Create.h"
#import "AppDelegate.h"
#import "AchievementModalViewController.h"

@interface TabBarViewController ()
{
NSDictionary *dataTypes;
NSMutableDictionary *achievementData;
NSMutableArray *achievementsToUpdate;
}

@property (strong, nonatomic) HKHealthStore *healthStore;

@end

@implementation TabBarViewController

@synthesize homeButton, settingsButton, achievementsButton, profileButton, gameCenterManager;

• (void)viewDidLoad {

[super viewDidLoad];
// Do any additional setup after loading the view.

self.selectedIndex = 3;

UIView *bar = [[[NSBundle mainBundle] loadNibNamed:@"CustomTabBar" owner:self options:nil] objectAtIndex:0];
bar.frame = CGRectMake(0, self.view.bounds.size.height - 49, self.view.bounds.size.width, 49);

[self.view addSubview:bar];

UIView *profileBorder = [[UIView alloc] initWithFrame:CGRectMake((self.view.frame.size.width / 4) - 1, 0, 1, 49)];
UIView *achievementsBorder = [[UIView alloc] initWithFrame:CGRectMake((self.view.frame.size.width / 4) - 1, 0, 1, 49)];
UIView *settingsBorder = [[UIView alloc] initWithFrame:CGRectMake((self.view.frame.size.width / 4) - 1, 0, 1, 49)];

profileBorder.backgroundColor = [UIColor colorWithHexString:@"e7e7e7"];
achievementsBorder.backgroundColor = [UIColor colorWithHexString:@"e7e7e7"];
settingsBorder.backgroundColor = [UIColor colorWithHexString:@"e7e7e7"];

[profileButton addSubview:profileBorder];
[achievementsButton addSu

You've posted a lot of code, so I'm going to give a general overview of some common mistakes I see people making (and are also happening here).

• You are using lots of magic numbers and magic strings. Let's instead create constants somewhere for all of these.

• You're using NSLog statements without #if DEBUG, which can be problematic.

• You have a lot of unnecessary vertical white space (empty lines). This decreases the readability when it's done to this degree.

• The word "and" should only be used if the method name is describing two separate actions.

• If all you're going to do is call super, just leave the method out entirely, otherwise it's clutter (didReceiveMemoryWarning).

Now, for some less common mistakes. Some advice slightly more specific to your code...

If looks like, at the end of the day, this is just a tab view controller with some slightly special code, right?

The IBAction methods at the bottom seem redundant... and it mostly seems like a default UITabViewController would handle these. We can subclass UITabViewController and put some authentication logic in it and let its default stuff handle a lot of stuff. There's nothign wrong with that.

if (difference > 0)
    existingDistance += (int)difference;

Where are your braces? If you must omit the braces, keep everything on one line, as such:

if (difference > 0) existingDistance += (int)difference;

But I must insist that you go ahead and add braces:

if (difference > 0) {
    existingDistance += (int)difference;
}

Your viewDidLoad method has way too much code in it. Moreover, it's very confusing that we keep switching back and forth between what view's we are setting up. Set one up completely, then set up the next completely, then the next, etc.

You flip flop back and forth between new and old style for things like NSDictionary.

myDict[@"myKey"] = someValue;

vs

[myDict setObject:someValue forKey:@"myKey"];

The former is vastly preferred to the latter.

We can also autobox NSNumber objects.

[NSNumber numberWithInt:1];

Could instead be simply:

@1

And this sort of thing is entirely unnecessary:

[defaults setObject:@"no" forKey:@"firstRun"];

NSUserDefaults has a setBool:forKey:, so if we need a boolean value, use that one:

[defaults setBool:NO forKey:@"firstRun"];

Finally, colorWithHexString: can be useful if we're fetching hex strings from a .plist file or downloading them from remote site, as hex strings are a fairly common means of storing colors. It may even be useful if we're letting the user enter a custom hex string for their color.

But if we're hard-coding it into our app, we should definitely not do this. Why would you want to make your app go through the hard work of parsing a string when we can just calculate ahead of time what these values should be and create the color using colorWithRed:green:blue:alpha:

StackExchange Code Review Q#91285, answer score: 4