Cross Check refactor

[gentilijuanmanuel]

Related issue

#8

Disclaimer

This PR should not be thought as a complete solution, it's a research and an explanation about the root cause of performance issues in Cross Check functionality, and it will involve technical and product input from people who know more about the project.

Summary

I found two problems in the code that runs when the user taps on "Cross Check" button on the right menu:

1. Async code misuse.
2. Expensive manipulations of the entries fetched from API and local DB.

1. Async code misuse

This is the code inside reloadData method, in OSTCrossCheckViewController.m, in master:

- (void) reloadData
{
    __block NSMutableArray * entriesThatShouldBeHere = [NSMutableArray new];
    [DejalBezelActivityView activityViewForView:self.view];
    
    dispatch_async(dispatch_get_main_queue(), ^{
        
        self.efforts = [EffortModel MR_findAllSortedBy:@"bibNumber" ascending:YES withPredicate:[NSPredicate predicateWithFormat:@"bibNumber != nil"]];
       
        [self fetchNotExpected];
         [self setFiltersQuantities];

            for (EffortModel * effort in self.efforts)
            {
                if ([effort checkIfEffortShouldBeInSplit:[CurrentCourse getCurrentCourse].splitName selectedSplitName:self.splitName])
                {
                    [effort expectedWithSplitName:self.splitName];
                    [entriesThatShouldBeHere addObject:effort];
                }
            }
            
            dispatch_async( dispatch_get_main_queue(), ^{
                self.efforts = entriesThatShouldBeHere;
                [self applyFilter];
               
                [DejalBezelActivityView removeViewAnimated:YES];
            });
    });
}

It:

• Creates a mutable array of efforts.
• Starts a loading indicator.
• Runs a completion block in the main queue. Inside that completion block, it:
  • Does a local database fetch with some filters.
  • Performs an API call (fetchNotExpected). The result of that API call is not awaited, that means, the API call starts, but the code continues with the next instruction (setFiltersQuantities) without waiting for that API call to finish with some result.
  • Does some manipulation and filters, trying to fill the mutable array created at the beggining.
  • Runs again a completion block in the main queue, which doesn't make sense, because we already are inside a completion block that is running in the main queue, and inside it it assigns the mutable array to the property efforts, applies some filters, and stops the loading indicator.

The main goal of the main queue is to perform UI related code. In general, a good practice related to concurrency is to free the main queue to only run UI related code, and try to perform heavy tasks (like API calls) in different threads, taking advantage of the advantage of a multithreading environment.
Because of that, and because this chunk of code was implemented a couple of years before the complete app codebase (for some reason I'm not able to see the PR), I suspect that there is some misuse of async code.

I refactored that code e little bit, to follow better practices (and also what's done in other parts of the app), but for that I assumed a couple of things:

• The code that runs after the API call (fetchNotExpected) should wait to the result of that call.
• Since the app is offline-first, if the API call fails because there is no internet connection, in the error completion block we should also perform the code after fetchNotExpected.

The result is the following:

• reloadData:

- (void) reloadData
{
    [self fetchNotExpected];
}

• fetchNotExpected:

- (void)fetchNotExpected
{
    [DejalBezelActivityView activityViewForView: self.view];
    
    self.efforts = [EffortModel MR_findAllSortedBy:@"bibNumber" ascending:YES withPredicate:[NSPredicate predicateWithFormat:@"bibNumber != nil"]];
    
    [[AppDelegate getInstance].getNetworkManager fetchNotExpected:[CurrentCourse getCurrentCourse].eventGroupId splitName:self.splitName useAlternateServer:NO completionBlock:^(id  _Nullable object) {
        [DejalBezelActivityView removeViewAnimated:YES];
        
        __block NSMutableArray * entriesThatShouldBeHere = [NSMutableArray new];
        
        if ([object isKindOfClass:[NSDictionary class]])
        {
            id bibNumbers = [object valueForKeyPath:@"data.bib_numbers"];
            if ([bibNumbers isKindOfClass:[NSArray class]])
            {
                [self bulkNotExpectedBibNumbers:bibNumbers];
                [self setFiltersQuantities];
                [self.crossCheckCollection reloadData];
            }
        }
        
        [self setFiltersQuantities];
        
        for (EffortModel * effort in self.efforts)
        {
            if ([effort checkIfEffortShouldBeInSplit:[CurrentCourse getCurrentCourse].splitName selectedSplitName:self.splitName])
            {
                [effort expectedWithSplitName:self.splitName];
                [entriesThatShouldBeHere addObject:effort];
            }
        }
        self.efforts = entriesThatShouldBeHere;
        [self applyFilter];
        
    } errorBlock:^(NSError * _Nullable error) {
        [DejalBezelActivityView removeViewAnimated:YES];
        
        __block NSMutableArray * entriesThatShouldBeHere = [NSMutableArray new];
        
        [self setFiltersQuantities];
        
        for (EffortModel * effort in self.efforts)
        {
            if ([effort checkIfEffortShouldBeInSplit:[CurrentCourse getCurrentCourse].splitName selectedSplitName:self.splitName])
            {
                [effort expectedWithSplitName:self.splitName];
                [entriesThatShouldBeHere addObject:effort];
            }
        }
        self.efforts = entriesThatShouldBeHere;
        [self applyFilter];
    }];
}

The API call is fired in a background queue, decreasing the amount of work in the main queue, and then the completion handlers (for success or error) are fired in the main queue, because they involve UI related stuff.
If you take a look at others API calls inside the codebase, they are following the same pattern. One example is this one.

2. Expensive manipulations of the entries fetched from API and local DB

In reloadData, the app is fetching data from API and from local DB. After that, it's performing a lot of manipulations to decide which efforts should it show in the collection view and which of them should filter out. There are a bunch of nested for statements to decide that. These manipulations are causing a severe hang in the main thread, as this screenshot of Xcode Instruments show:

I think we should try to understand the reason behind those nested fors in a better way in order to see if we can delete or merge some of them, to decrease the time complexity of the whole algorithm. But for that, we need to understand the feature itself and each entity involved here.