Race conditions in iOS using GCD for UICollectionView

Question

I am hoping that someone can help me out with this code and try to figure out the best way to optimize it so I don't have so many race conditions.

Some things that I know are happening:

• Sometimes the collection view does not even show up
• Only some items show up in collection view i.e 3 out of 8 items
• The refresh graph function gets called anywhere from 1-3 times randomly

I just think overall this code can be made more efficient. If anyone has any advice, I would greatly appreciate it.

I am going to post the bulk of the code here, I have removed a lot of processing to save lines so it isn't so hard to read but I think I left in enough so you can see where GCD is being utilized and if I'm doing it wrong. I will also link the full code in a gist if you would like to see the whole thing.

Gist

#import "DashboardCollectionViewController.h"
#import "DashboardLayout.h"
#import "HeaderViewCell.h"
#import "DashboardCell.h"
#import "DashboardDetailViewController.h"
#import "PreferencesManager.h"
#import "UIColor+HexColors.h"
#import "PNChart.h"

@interface DashboardCollectionViewController () <UICollectionViewDelegateFlowLayout, UICollectionViewDataSource, UICollectionViewDelegate>

@property (nonatomic, strong) DashboardLayout *listLayout;
@property (nonatomic, strong) DashboardLayout *gridLayout;
@property (strong, nonatomic) HKHealthStore *healthStore;
@property (strong, nonatomic) NSDictionary *dataTypes;
@property (strong, nonatomic) HeaderViewCell *headerCell;

@end

@implementation DashboardCollectionViewController
{

    NSString *layoutType;
    NSString *unitPreference;
    NSMutableDictionary *itemData;
    UIRefreshControl *refreshControl;
    NSArray *sortedDashboardItems;
    __block NSMutableArray *graphData;

}

@synthesize dashboardItems, allDashboardItems, myCollectionView, dataTypes, headerCell;

- (void)viewDidLoad {

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

    [self setupFlowLayoutA];
    [self setupFlowLayoutB];

    layoutType = @"listView";
    dataTypes = [NSDictionary dictionaryWithObjectsAndKeys:
                 @1, HKQuantityTypeIdentifierStepCount,
                 @2, HKQuantityTypeIdentifierFlightsClimbed,
                 @3, HKQuantityTypeIdentifierDistanceWalkingRunning,
                 @4, HKQuantityTypeIdentifierActiveEnergyBurned,
                 @5, HKQuantityTypeIdentifierBodyMass,
                 @6, HKQuantityTypeIdentifierDistanceCycling,
                 @7, HKQuantityTypeIdentifierHeartRate,
                 @8, HKQuantityTypeIdentifierBodyMassIndex, nil];

    UINib *headerNib = [UINib nibWithNibName:@"HeaderView" bundle:nil];
    UINib *cellNib = [UINib nibWithNibName:@"DashboardCell" bundle:nil];
    DashboardLayout *dashboardLayout = [self listLayout];

    [myCollectionView registerNib:headerNib forSupplementaryViewOfKind:UICollectionElementKindSectionHeader withReuseIdentifier:@"HeaderCell"];
    [myCollectionView registerNib:cellNib forCellWithReuseIdentifier:@"Cell"];
    [myCollectionView setCollectionViewLayout:dashboardLayout animated:YES];

    //check if health kit is available on this device
    if ([HKHealthStore isHealthDataAvailable]) {

        if (!self.healthStore) {
            self.healthStore = [HKHealthStore new];
        }
    }

    refreshControl = [UIRefreshControl new];
    [refreshControl addTarget:self action:@selector(refreshAllData) forControlEvents:UIControlEventValueChanged];
    [myCollectionView addSubview:refreshControl];

}

- (void)viewWillAppear:(BOOL)animated {

    [super viewWillAppear:animated];

    //set data types we want to read and write
    NSSet *dataTypesToWrite = [self dataTypesToWrite];
    NSSet *datatTypesToRead = [self dataTypesToRead];

    allDashboardItems = [self loadDashboardItems];
    unitPreference = [self loadUnitPreferences];
    dashboardItems = [NSMutableArray new];
    itemData = [NSMutableDictionary new];

    for (NSDictionary *item in allDashboardItems) {

        NSString *enabled = item[@"enabled"];

        if ([enabled isEqualToString:@"1"]) {
            [dashboardItems addObject:item];
        }

    }

    NSSortDescriptor *descriptor = [[NSSortDescriptor alloc] initWithKey:@"order" ascending:YES];
    sortedDashboardItems = [dashboardItems sortedArrayUsingDescriptors:@[descriptor]];

    graphData = [[NSMutableArray alloc] initWithCapacity:[sortedDashboardItems count]];

    for (int i=0; i < [sortedDashboardItems count]; i++) {

        [graphData addObject:[NSNull null]];

    }

    NSUserDefaults *defaults = [NSUserDefaults standardUserDefaults];
#if DEBUG
    NSLog(@"%@", [defaults objectForKey:@"unitPreference"]);
#endif

    //request authorization
    [self.healthStore requestAuthorizationToShareTypes:dataTypesToWrite readTypes:datatTypesToRead completion:^(BOOL success, NSError *error) {
        if (!success) {
            //user did not authorize healthkit
            NSLog(@"Health Kit was not given the correct permissions");
            return;
        } else {

            [self refreshData];
            [self refreshGraphs];
           // [myCollectionView reloadData];

        }
    }];

    [self refreshData];
    [self refreshGraphs];
//    [myCollectionView reloadData];

}

- (void)refreshAllData {

    [self refreshData];
    [self refreshGraphs];

}

- (void)refreshData {

    __block NSString *labelString = @"";
    __block NSString *unitString = @"";

    NSCalendar *calendar = [NSCalendar currentCalendar];
    NSDate *startDate = [calendar startOfDayForDate:[NSDate date]];
    NSDate *endDate = [calendar dateByAddingUnit:NSCalendarUnitDay value:1 toDate:startDate options:0];
    NSPredicate *predicate = [HKQuery predicateForSamplesWithStartDate:startDate endDate:endDate options:HKQueryOptionNone];

    for (NSDictionary *item in sortedDashboardItems) {

        NSString *type = item[@"type"];

        HKSampleType *sampleType = [HKSampleType quantityTypeForIdentifier:type];

        HKSampleQuery *query = [[HKSampleQuery alloc] initWithSampleType:sampleType predicate:predicate limit:HKObjectQueryNoLimit sortDescriptors:nil resultsHandler:^(HKSampleQuery *query, NSArray *results, NSError *error) {

            if (!results) {

                NSLog(@"No results were returned form query");

            } else if (error) {

                NSLog(@"Error: %@ %@", error, [error userInfo]);

            } else {

                dispatch_async(dispatch_get_main_queue(), ^{

                    //processing

                    int order = [item[@"order"] intValue];
                    NSNumber *orderNumber = [NSNumber numberWithInt:order];

                    UIFont *arialLarge = [UIFont fontWithName:@"AvenirNext-Bold" size:15.0];
                    UIFont *arialSmall = [UIFont fontWithName:@"AvenirNext-Bold" size:8.0];
                    NSDictionary *arialLargeDict = @{NSFontAttributeName : arialLarge};
                    NSDictionary *arialSmallDict = @{NSFontAttributeName : arialSmall};

                    NSMutableAttributedString *largeString = [[NSMutableAttributedString alloc] initWithString:labelString attributes:arialLargeDict];
                    NSMutableAttributedString *smallString = [[NSMutableAttributedString alloc] initWithString:unitString attributes:arialSmallDict];

                    [largeString appendAttributedString:smallString];

                    [itemData setObject:largeString forKey:orderNumber];

                    [myCollectionView reloadData];
                    [refreshControl endRefreshing];

                });

            }

        }];

        [self.healthStore executeQuery:query];

    }

}

- (void)refreshGraphs {

#if DEBUG
    NSLog(@"graphs");
#endif
    NSCalendar *calendar = [NSCalendar currentCalendar];
    NSDateComponents *interval = [NSDateComponents new];
    interval.day = 1;

    NSDate *anchorDate = [calendar dateByAddingUnit:NSCalendarUnitDay value:-6 toDate:[calendar startOfDayForDate:[NSDate date]] options:0];

    dispatch_queue_t queue = dispatch_queue_create([@"graph.queue" UTF8String], DISPATCH_QUEUE_CONCURRENT);
    dispatch_group_t group = dispatch_group_create();

    for (NSDictionary *item in sortedDashboardItems) {

        NSMutableArray *arrayOfValues = [NSMutableArray new];

        NSString *type = item[@"type"];

        NSUInteger index = [sortedDashboardItems indexOfObject:item];

        HKQuantityType *quantityType = [HKObjectType quantityTypeForIdentifier:type];

        NSDate *endDate = [NSDate date];

        NSDate *startDate = [calendar dateByAddingUnit:NSCalendarUnitDay value:-6 toDate:[calendar startOfDayForDate:endDate] options:0];

        dispatch_block_t block = ^{

            dispatch_semaphore_t lock = dispatch_semaphore_create(0);

            if ([type isEqualToString:HKQuantityTypeIdentifierBodyMass]) {

                HKStatisticsCollectionQuery *query = [[HKStatisticsCollectionQuery alloc] initWithQuantityType:quantityType quantitySamplePredicate:nil options:HKStatisticsOptionDiscreteAverage anchorDate:anchorDate intervalComponents:interval];

                query.initialResultsHandler = ^(HKStatisticsCollectionQuery *query, HKStatisticsCollection *results, NSError *error) {

                    //processing

                };

                [self.healthStore executeQuery:query];

                dispatch_semaphore_wait(lock, DISPATCH_TIME_FOREVER);

            } else if ([type isEqualToString:HKQuantityTypeIdentifierBodyMassIndex]) {

                HKStatisticsCollectionQuery *query = [[HKStatisticsCollectionQuery alloc] initWithQuantityType:quantityType quantitySamplePredicate:nil options:HKStatisticsOptionDiscreteAverage anchorDate:anchorDate intervalComponents:interval];

                query.initialResultsHandler = ^(HKStatisticsCollectionQuery *query, HKStatisticsCollection *results, NSError *error) {

                    //processing

                };

                [self.healthStore executeQuery:query];

                dispatch_semaphore_wait(lock, DISPATCH_TIME_FOREVER);

            } else if ([type isEqualToString:HKQuantityTypeIdentifierHeartRate]) {

                HKStatisticsCollectionQuery *query = [[HKStatisticsCollectionQuery alloc] initWithQuantityType:quantityType quantitySamplePredicate:nil options:HKStatisticsOptionDiscreteAverage anchorDate:anchorDate intervalComponents:interval];

                query.initialResultsHandler = ^(HKStatisticsCollectionQuery *query, HKStatisticsCollection *results, NSError *error) {

                    //processing

                };

                [self.healthStore executeQuery:query];

                dispatch_semaphore_wait(lock, DISPATCH_TIME_FOREVER);

            } else {

                HKStatisticsCollectionQuery *query = [[HKStatisticsCollectionQuery alloc] initWithQuantityType:quantityType quantitySamplePredicate:nil options:HKStatisticsOptionCumulativeSum anchorDate:anchorDate intervalComponents:interval];

                query.initialResultsHandler = ^(HKStatisticsCollectionQuery *query, HKStatisticsCollection *results, NSError *error) {

                    if (error) {

                        NSLog(@"Error: %@ %@", error, [error userInfo]);

                    } else {

                        [results enumerateStatisticsFromDate:startDate toDate:endDate withBlock:^(HKStatistics *result, BOOL *stop) {

                            HKQuantity *quantity = result.sumQuantity;

                            if (quantity != nil) {

                                //do some processing

                            } else {

                                [arrayOfValues addObject:@0];

                            }

                        }];

                        [self addArrayToGraphData:arrayOfValues atIndex:index];

                        dispatch_semaphore_signal(lock);

                    }

                };

                [self.healthStore executeQuery:query];

                dispatch_semaphore_wait(lock, DISPATCH_TIME_FOREVER);

            };

        };

        dispatch_group_async(group, queue, block);

    }

    dispatch_group_notify(group, queue, ^{

        [myCollectionView reloadData];

    });

}

- (void)addArrayToGraphData:(NSArray *)array atIndex:(NSUInteger)index {

    [graphData replaceObjectAtIndex:index withObject:array];

}

- (UICollectionViewCell *)collectionView:(UICollectionView *)collectionView cellForItemAtIndexPath:(NSIndexPath *)indexPath {

    DashboardCell *cell = [collectionView dequeueReusableCellWithReuseIdentifier:@"Cell" forIndexPath:indexPath];

    if (cell != nil) {

        for (UIView *subview in cell.subviews) {

            if ([subview isKindOfClass:[PNLineChart class]]) {

                [subview removeFromSuperview];

            }

        }

        NSNumber *indexNumber = [NSNumber numberWithInteger:indexPath.row];
        NSDictionary *item = [sortedDashboardItems objectAtIndex:indexPath.row];
        NSString *imageSafeString = [item[@"title"] stringByReplacingOccurrencesOfString:@" " withString:@""];
        NSString *imageString = @"";

        if ([layoutType isEqualToString:@"listView"]) {

            imageString = [NSString stringWithFormat:@"%@-large.png", imageSafeString];
            cell.layoutType = layoutType;
            cell.titleLabel.text = item[@"title"];
            cell.dataImage.image = [UIImage imageNamed:imageString];

        } else if ([layoutType isEqualToString:@"gridView"]) {

            imageString = [NSString stringWithFormat:@"%@-small.png", imageSafeString];
            cell.layoutType = layoutType;
            cell.titleLabel.text = @"";
            cell.dataImage.image = [UIImage imageNamed:imageString];

        }

        PNLineChart *graph = nil;

        if ([layoutType isEqualToString:@"listView"]) {

            graph = [[PNLineChart alloc] initWithFrame:CGRectMake(80, 50, self.view.frame.size.width - 100, 90)];

        } else {

            graph = [[PNLineChart alloc] initWithFrame:CGRectMake(10, 0, (self.view.frame.size.width / 2) - 50, 110)];

        }

        graph.showLabel = NO;
        graph.showGenYLabels = NO;
        graph.backgroundColor = [UIColor clearColor];
        graph.pointColor = [UIColor whiteColor];
        graph.userInteractionEnabled = NO;

        [cell addSubview:graph];

        PNLineChartData *data = [PNLineChartData new];
        data.color = [UIColor colorWithHexString:@"49b1d9"];
        if (![graphData[indexPath.row] isEqual:[NSNull null]]) {
            data.itemCount = [graphData[indexPath.row] count];
        } else {
            data.itemCount = 0;
        }
        data.inflexionPointStyle = PNLineChartPointStyleCircle;
        data.getData = ^(NSUInteger index) {

            CGFloat yValue = [graphData[indexPath.row][index] floatValue];
            return [PNLineChartDataItem dataItemWithY:yValue];

        };

        graph.chartData = @[data];
        [graph strokeChart];

        NSAttributedString *dataString = [itemData objectForKey:indexNumber];

        cell.dataLabel.attributedText = dataString;

    }

    return cell;

}

Answer 1

It seems like the code is too complex for any one to look at it and guess what actually goes on. Would it be nice if you separated it into some proper model or service layer, such that the code doing presentation and the code fetching data from healthkit would be separate.

I clearly see the threading issue here, first and foremost is the usage of the HKHealthStore method,

- (void)requestAuthorizationToShareTypes:(NSSet *)typesToShare
                               readTypes:(NSSet *)typesToRead
                              completion:(void (^)(BOOL success,
                                                   NSError *error))completion 

It is quite clearly dictated in documentation that this method run and the callback is called on some arbitrary queue,

https://developer.apple.com/library/prerelease/ios/documentation/HealthKit/Reference/HKHealthStore_Class/index.html#//apple_ref/occ/instm/HKHealthStore/requestAuthorizationToShareTypes:readTypes:completion:

HealthKit performs these requests asynchronously. If you call this method with a new data type (a type of data that the user has not previously granted or denied permission for in this app), the system automatically displays the permission form, listing all the requested permissions. After the user has finished responding, this method calls its completion block on a background queue. If the user has already chosen to grant or prohibit access to all of the types specified, the completion is called without prompting the user.

Now, are you really sure that you are doing all your UI call in the main thread. I think there are multiple asynchronous queries that you are executing. So, doesnot dispatch_semaphore look complex in this case. Would it make more sense to use dispatch_group_enter and dispatch_group_leave to manage the concurrent tasks. I dont see that you are reloading the collectionview in the main thread. Those are the main issues, that I see from the code above. You would save tonns of time refactoring and figuring out bugs later if you pick up the service code to some different objects.