Refactor until you can Maintain

Almost every software developer maintains code one way or the other. But, does every software developer write maintainable code? Answering that question would definitely go beyond the scope of this blog. Instead, I would like to give an example from my day-to-day work about the importance of maintainable code.

I am currently working on an iOS project and I was recently given the task to fix an issue about the Core Data migration. The problem was that the Core Data database was wiped-out when the users upgraded to a new version of the app that contains an updated Core Data model. First thing I did was to find the relevant piece of code in the project, and there it was:

 1
 2
 3
 4
 5
 6
 7
 8
 9
10
11
12
13
14
15
16
17
18
19
20
21
22
23
24
25
26
27
28
29
30
31
32
33
34
35
36
37
38
39
40
41
42
43
44
45
46
47
48
49
50
51
52
53
54
55
56
57
58
59
60
61
62
63
64
65
66
67
68
69
70
71
72
73
74
75
76
77
78
79
80
81
82
83
84
85
- (NSPersistentStoreCoordinator *)persistentStoreCoordinator
{
    if (_persistentStoreCoordinator != nil) {
        return _persistentStoreCoordinator;
    }
    
    id appDelegate = [[UIApplication sharedApplication] delegate];
    NSString *storeName = @"MyApp.sqlite";
    NSURL *storeURL = [[appDelegate applicationCachesDirectory] URLByAppendingPathComponent:storeName];
    
    NSError *error = nil;
    _persistentStoreCoordinator = [[NSPersistentStoreCoordinator alloc] initWithManagedObjectModel:[self managedObjectModel]];
    
    
    NSDictionary *sourceMetadata = [NSPersistentStoreCoordinator metadataForPersistentStoreOfType:nil
                                                                                              URL:storeURL
                                                                                            error:&error];
    if (sourceMetadata == nil) {
         // deal with error
        DDLogDebug(@"Error: no metatada %s", __PRETTY_FUNCTION__);
     }
    
    
    NSManagedObjectModel *destinationModel = [_persistentStoreCoordinator managedObjectModel];

    NSDictionary *options = @{NSMigratePersistentStoresAutomaticallyOption: @YES,
                              NSInferMappingModelAutomaticallyOption: @YES,
                              NSSQLitePragmasOption: @{@"journal_mode": @"WAL"}};

    
    BOOL pscCompatibile = [destinationModel isConfiguration:nil
                                compatibleWithStoreMetadata:sourceMetadata];
    
    if (!pscCompatibile) {
        NSString *newstoreName = @"MyAppNew.sqlite”;
        NSURL *dstStoreURL = [[appDelegate applicationCachesDirectory] URLByAppendingPathComponent:newstoreName];

        error = nil;
        
        [[NSFileManager defaultManager] removeItemAtURL:dstStoreURL
                                                  error:nil];
        
        BOOL success = [self migrateStore:storeURL
                        toVersionTwoStore:dstStoreURL
                                    error:&error];
        
        if (success) {
            DDLogVerbose(@"store converted from %@ to %@", storeURL, dstStoreURL);
            [[NSFileManager defaultManager] moveItemAtURL:dstStoreURL
                                                    toURL:storeURL
                                                    error:nil];
            
            NSString *message = NSLocalizedString(@"Migrated database to new version", @"Migrated database to new version");
            [[NotificationWindow sharedInstance] presentNotification:NotificationTypeSuccess message:message];
        } else {
            NSString *message = NSLocalizedString(@"Failed database conversion", @"Failed database conversion");
            [[NotificationWindow sharedInstance] presentNotification:NotificationTypeError message:message];

            DDLogVerbose(@"FAILED store conversion from %@ to %@", storeURL, dstStoreURL);
        }

    }
    
    if (![_persistentStoreCoordinator addPersistentStoreWithType:NSSQLiteStoreType configuration:nil URL:storeURL options:options error:&error]) {
        //migration failed, we need to delete
        NSError *contentsError = nil;
        NSArray *directoryContents = [[NSFileManager defaultManager] contentsOfDirectoryAtURL:[appDelegate applicationCachesDirectory]
                                                                   includingPropertiesForKeys:@[(NSString *)kCFURLNameKey]
                                                                                      options:NSDirectoryEnumerationSkipsSubdirectoryDescendants |NSDirectoryEnumerationSkipsPackageDescendants | NSDirectoryEnumerationSkipsHiddenFiles
                                                                                        error:&contentsError];
        for (NSURL *fileUrlToDelete in directoryContents) {
            NSString *fileName = [fileUrlToDelete.path lastPathComponent];
            if ([fileName hasPrefix:storeName]) {
                [[NSFileManager defaultManager] removeItemAtURL:fileUrlToDelete error:nil];
            }
        }
        
        if (![_persistentStoreCoordinator addPersistentStoreWithType:NSSQLiteStoreType configuration:nil URL:storeURL options:options error:&error]) {
            NSLog(@"ERROR: %@", error);
            return nil;
        }
    }
    
    return _persistentStoreCoordinator;
}

It is not very good news when you see that this is the piece of code that you will be fixing. 90 lines of code in one big method (I know that it could have been much worse, we probably all have worked on such code). First thing I had to do was to refactor the code. Here is how I approached it step-by-step:

1) Simply define the variables close to where you will be using them. If there is a variable that has been used throughout the whole method, put that variable declaration to where it was used for the first time. The code then looked like:

 1
 2
 3
 4
 5
 6
 7
 8
 9
10
11
12
13
14
15
16
17
18
19
20
21
22
23
24
25
26
27
28
29
30
31
32
33
34
35
36
37
38
39
40
41
42
43
44
45
46
47
48
49
50
51
52
53
54
55
56
57
58
59
60
61
62
63
64
65
66
67
68
69
70
71
72
73
74
75
76
77
78
79
80
81
82
- (NSPersistentStoreCoordinator *)persistentStoreCoordinator2
{
    if (_persistentStoreCoordinator != nil) {
        return _persistentStoreCoordinator;
    }
    
    id appDelegate = [[UIApplication sharedApplication] delegate];
    NSString *storeName = @"MyApp.sqlite";
    NSURL *storeURL = [[appDelegate applicationCachesDirectory] URLByAppendingPathComponent:storeName];
    
    
    _persistentStoreCoordinator = [[NSPersistentStoreCoordinator alloc] initWithManagedObjectModel:[self managedObjectModel]];
    
    
    NSError *error = nil;
    NSDictionary *sourceMetadata = [NSPersistentStoreCoordinator metadataForPersistentStoreOfType:nil
                                                                                              URL:storeURL
                                                                                            error:&error];
    if (sourceMetadata == nil) {
        // deal with error
        DDLogDebug(@"Error: no metatada %s", __PRETTY_FUNCTION__);
    }
    
    
    NSManagedObjectModel *destinationModel = [_persistentStoreCoordinator managedObjectModel];
    BOOL pscCompatibile = [destinationModel isConfiguration:nil
                                compatibleWithStoreMetadata:sourceMetadata];
    
    if (!pscCompatibile) {
        NSString *newstoreName = @"MyAppNew.sqlite";
        NSURL *dstStoreURL = [[appDelegate applicationCachesDirectory] URLByAppendingPathComponent:newstoreName];
        
        [[NSFileManager defaultManager] removeItemAtURL:dstStoreURL
                                                  error:nil];
        
        error = nil;
        BOOL success = [self migrateStore:storeURL
                        toVersionTwoStore:dstStoreURL
                                    error:&error];
        
        if (success) {
            DDLogVerbose(@"store converted from %@ to %@", storeURL, dstStoreURL);
            [[NSFileManager defaultManager] moveItemAtURL:dstStoreURL
                                                    toURL:storeURL
                                                    error:nil];
            
            NSString *message = NSLocalizedString(@"Migrated database to new version", @"Migrated database to new version");
            [[NotificationWindow sharedInstance] presentNotification:NotificationTypeSuccess message:message];
        } else {
            NSString *message = NSLocalizedString(@"Failed database conversion", @"Failed database conversion");
            [[NotificationWindow sharedInstance] presentNotification:NotificationTypeError message:message];
            
            DDLogVerbose(@"FAILED store conversion from %@ to %@", storeURL, dstStoreURL);
        }
        
    }
    
    NSDictionary *options = @{NSMigratePersistentStoresAutomaticallyOption: @YES,
                              NSInferMappingModelAutomaticallyOption: @YES,
                              NSSQLitePragmasOption: @{@"journal_mode": @"WAL"}};
    if (![_persistentStoreCoordinator addPersistentStoreWithType:NSSQLiteStoreType configuration:nil URL:storeURL options:options error:&error]) {
        //migration failed, we need to delete
        NSError *contentsError = nil;
        NSArray *directoryContents = [[NSFileManager defaultManager] contentsOfDirectoryAtURL:[appDelegate applicationCachesDirectory]
                                                                   includingPropertiesForKeys:@[(NSString *)kCFURLNameKey]
                                                                                      options:NSDirectoryEnumerationSkipsSubdirectoryDescendants |NSDirectoryEnumerationSkipsPackageDescendants | NSDirectoryEnumerationSkipsHiddenFiles
                                                                                        error:&contentsError];
        for (NSURL *fileUrlToDelete in directoryContents) {
            NSString *fileName = [fileUrlToDelete.path lastPathComponent];
            if ([fileName hasPrefix:storeName]) {
                [[NSFileManager defaultManager] removeItemAtURL:fileUrlToDelete error:nil];
            }
        }
        
        if (![_persistentStoreCoordinator addPersistentStoreWithType:NSSQLiteStoreType configuration:nil URL:storeURL options:options error:&error]) {
            NSLog(@"ERROR: %@", error);
            return nil;
        }
    }
    
    return _persistentStoreCoordinator;
}


2) Find any duplicate code and extract it out to its own method. Fortunately, there was no code duplication in this method; already a big plus for code maintenance.

3) This is my favorite step: Break down the method into meaningful pieces of functionality. You don’t have to get everything right all at once, simply try to isolate the different blocks of functionality. Here is how the code looked after:

  1
  2
  3
  4
  5
  6
  7
  8
  9
 10
 11
 12
 13
 14
 15
 16
 17
 18
 19
 20
 21
 22
 23
 24
 25
 26
 27
 28
 29
 30
 31
 32
 33
 34
 35
 36
 37
 38
 39
 40
 41
 42
 43
 44
 45
 46
 47
 48
 49
 50
 51
 52
 53
 54
 55
 56
 57
 58
 59
 60
 61
 62
 63
 64
 65
 66
 67
 68
 69
 70
 71
 72
 73
 74
 75
 76
 77
 78
 79
 80
 81
 82
 83
 84
 85
 86
 87
 88
 89
 90
 91
 92
 93
 94
 95
 96
 97
 98
 99
100
101
102
103
104
105
106
107
108
109
110
111
112
- (NSDictionary*) sourceMetadata {

    id appDelegate = [[UIApplication sharedApplication] delegate];
    NSString *storeName = @"MyApp.sqlite";
    NSURL *storeURL = [[appDelegate applicationCachesDirectory] URLByAppendingPathComponent:storeName];
    
    NSError *error = nil;
    NSDictionary* sourceMetadata = [NSPersistentStoreCoordinator metadataForPersistentStoreOfType:nil
                                                                                              URL:storeURL
                                                                                            error:&error];
    if (sourceMetadata == nil) {
        // deal with error
        DDLogDebug(@"Error: no metatada %s", __PRETTY_FUNCTION__);
    }
    
    return sourceMetadata;
}

- (void) migrateStore {

    id appDelegate = [[UIApplication sharedApplication] delegate];
    NSString *storeName = @"MyApp.sqlite";
    NSURL *storeURL = [[appDelegate applicationCachesDirectory] URLByAppendingPathComponent:storeName];
    NSString *newstoreName = @"MyAppNew.sqlite";
    NSURL *dstStoreURL = [[appDelegate applicationCachesDirectory] URLByAppendingPathComponent:newstoreName];
    
    [[NSFileManager defaultManager] removeItemAtURL:dstStoreURL
                                              error:nil];
    
    NSError *error = nil;
    BOOL success = [self migrateStore:storeURL
                    toVersionTwoStore:dstStoreURL
                                error:&error];
    
    if (success) {
        DDLogVerbose(@"store converted from %@ to %@", storeURL, dstStoreURL);
        [[NSFileManager defaultManager] moveItemAtURL:dstStoreURL
                                                toURL:storeURL
                                                error:nil];
        
        NSString *message = NSLocalizedString(@"Migrated database to new version", @"Migrated database to new version");
        [[NotificationWindow sharedInstance] presentNotification:NotificationTypeSuccess message:message];
    } else {
        NSString *message = NSLocalizedString(@"Failed database conversion", @"Failed database conversion");
        [[NotificationWindow sharedInstance] presentNotification:NotificationTypeError message:message];
        
        DDLogVerbose(@"FAILED store conversion from %@ to %@", storeURL, dstStoreURL);
    }
}

- (void) removeOldStoreCompletelyAndAddTheNewOne {

    id appDelegate = [[UIApplication sharedApplication] delegate];
    NSString *storeName = @"MyApp.sqlite";
    NSURL *storeURL = [[appDelegate applicationCachesDirectory] URLByAppendingPathComponent:storeName];
    NSError* error = nil;
    
    //migration failed, we need to delete
    NSError *contentsError = nil;
    NSArray *directoryContents = [[NSFileManager defaultManager] contentsOfDirectoryAtURL:[appDelegate applicationCachesDirectory]
                                                               includingPropertiesForKeys:@[(NSString *)kCFURLNameKey]
                                                                                  options:NSDirectoryEnumerationSkipsSubdirectoryDescendants |NSDirectoryEnumerationSkipsPackageDescendants | NSDirectoryEnumerationSkipsHiddenFiles
                                                                                    error:&contentsError];
    
    
    for (NSURL *fileUrlToDelete in directoryContents) {
        NSString *fileName = [fileUrlToDelete.path lastPathComponent];
        if ([fileName hasPrefix:storeName]) {
            [[NSFileManager defaultManager] removeItemAtURL:fileUrlToDelete error:nil];
        }
    }
    
    NSDictionary *options = @{NSMigratePersistentStoresAutomaticallyOption: @YES,
                              NSInferMappingModelAutomaticallyOption: @YES,
                              NSSQLitePragmasOption: @{@"journal_mode": @"WAL"}};
    if (![_persistentStoreCoordinator addPersistentStoreWithType:NSSQLiteStoreType configuration:nil URL:storeURL options:options error:&error]) {
        NSLog(@"ERROR: %@", error);
    }
}

- (NSPersistentStoreCoordinator *)persistentStoreCoordinator3
{
    if (_persistentStoreCoordinator != nil) {
        return _persistentStoreCoordinator;
    }
    
    _persistentStoreCoordinator = [[NSPersistentStoreCoordinator alloc] initWithManagedObjectModel:[self managedObjectModel]];
    
    NSDictionary* sourceMetadata = [self sourceMetadata];
    
    NSManagedObjectModel *destinationModel = [_persistentStoreCoordinator managedObjectModel];
    BOOL pscCompatibile = [destinationModel isConfiguration:nil
                                compatibleWithStoreMetadata:sourceMetadata];
    
    if (!pscCompatibile) {
        [self migrateStore];
    }
    
    id appDelegate = [[UIApplication sharedApplication] delegate];
    NSString *storeName = @"MyApp.sqlite";
    NSURL *storeURL = [[appDelegate applicationCachesDirectory] URLByAppendingPathComponent:storeName];
    NSError* error = nil;
    NSDictionary *options = @{NSMigratePersistentStoresAutomaticallyOption: @YES,
                              NSInferMappingModelAutomaticallyOption: @YES,
                              NSSQLitePragmasOption: @{@"journal_mode": @"WAL"}};
    
    if (![_persistentStoreCoordinator addPersistentStoreWithType:NSSQLiteStoreType configuration:nil URL:storeURL options:options error:&error]) {
        [self removeOldStoreCompletelyAndAddTheNewOne];
    }
    
    return _persistentStoreCoordinator;
}


4) The main method looks a lot better now. It is shorter and a lot more readable. However, last step was a bit controversial. I have actually introduced code duplication with this step; the code to retrieve the store URLs and app delegate are repeated several times. That is fine; it is part of the iterative refactoring process. I will now extract those functionalities to their own methods and do a little clean-up:


  1
  2
  3
  4
  5
  6
  7
  8
  9
 10
 11
 12
 13
 14
 15
 16
 17
 18
 19
 20
 21
 22
 23
 24
 25
 26
 27
 28
 29
 30
 31
 32
 33
 34
 35
 36
 37
 38
 39
 40
 41
 42
 43
 44
 45
 46
 47
 48
 49
 50
 51
 52
 53
 54
 55
 56
 57
 58
 59
 60
 61
 62
 63
 64
 65
 66
 67
 68
 69
 70
 71
 72
 73
 74
 75
 76
 77
 78
 79
 80
 81
 82
 83
 84
 85
 86
 87
 88
 89
 90
 91
 92
 93
 94
 95
 96
 97
 98
 99
100
101
102
103
104
105
106
107
108
109
110
111
112
113
114
115
116
117
118
119
120
- (id) appDelegate { return [[UIApplication sharedApplication] delegate]; }

- (NSString*) previousStoreName { return VERSION_1_STORE_NAME; }
- (NSString*) currentStoreName { return VERSION_2_STORE_NAME; }

- (NSURL*) previousStoreURL {
    NSURL* directoryUrlForStores = [[self appDelegate] applicationCachesDirectory];
    return [directoryUrlForStores URLByAppendingPathComponent:[self previousStoreName]];
}

- (NSURL*) currentStoreURL {
    NSURL* directoryUrlForStores = [[self appDelegate] applicationCachesDirectory];
    return [directoryUrlForStores URLByAppendingPathComponent:[self currentStoreName]];
}

- (NSDictionary*) sourceMetadata {
    
    NSURL *storeURL = [self previousStoreURL];
    
    NSError *error = nil;
    NSDictionary* sourceMetadata = [NSPersistentStoreCoordinator metadataForPersistentStoreOfType:nil
                                                                                              URL:storeURL
                                                                                            error:&error];
    if (sourceMetadata == nil) {
        // deal with error
        DDLogDebug(@"Error: no metatada %s", __PRETTY_FUNCTION__);
    }
    
    return sourceMetadata;
}

- (void) migrateStore {
    
    id appDelegate = [self appDelegate];
    NSURL *storeURL = [self previousStoreURL];
    NSURL* newStoreURL = [self currentStoreURL];
    
    [[NSFileManager defaultManager] removeItemAtURL:newStoreURL
                                              error:nil];
    
    NSError *error = nil;
    BOOL success = [self migrateStore:storeURL
                    toVersionTwoStore:newStoreURL
                                error:&error];
    
    if (success) {
        DDLogVerbose(@"store converted from %@ to %@", storeURL, newStoreURL);
        [[NSFileManager defaultManager] moveItemAtURL:newStoreURL
                                                toURL:storeURL
                                                error:nil];
        
        NSString *message = NSLocalizedString(@"Migrated database to new version", @"Migrated database to new version");
        [[NotificationWindow sharedInstance] presentNotification:NotificationTypeSuccess message:message];
    } else {
        NSString *message = NSLocalizedString(@"Failed database conversion", @"Failed database conversion");
        [[NotificationWindow sharedInstance] presentNotification:NotificationTypeError message:message];
        
        DDLogVerbose(@"FAILED store conversion from %@ to %@", storeURL, newStoreURL);
    }
}

- (void) removeOldStoreCompletelyAndAddTheNewOne {
    
    id appDelegate = [self appDelegate];
    NSURL *storeURL = [self previousStoreURL];
    NSError* error = nil;
    
    //migration failed, we need to delete
    NSError *contentsError = nil;
    NSArray *directoryContents = [[NSFileManager defaultManager] contentsOfDirectoryAtURL:[appDelegate applicationCachesDirectory]
                                                               includingPropertiesForKeys:@[(NSString *)kCFURLNameKey]
                                                                                  options:NSDirectoryEnumerationSkipsSubdirectoryDescendants |NSDirectoryEnumerationSkipsPackageDescendants | NSDirectoryEnumerationSkipsHiddenFiles
                                                                                    error:&contentsError];
    
    
    for (NSURL *fileUrlToDelete in directoryContents) {
        NSString *fileName = [fileUrlToDelete.path lastPathComponent];
        if ([fileName hasPrefix:VERSION_1_STORE_NAME]) {
            [[NSFileManager defaultManager] removeItemAtURL:fileUrlToDelete error:nil];
        }
    }
    
    NSDictionary *options = @{NSMigratePersistentStoresAutomaticallyOption: @YES,
                              NSInferMappingModelAutomaticallyOption: @YES,
                              NSSQLitePragmasOption: @{@"journal_mode": @"WAL"}};
    if (![_persistentStoreCoordinator addPersistentStoreWithType:NSSQLiteStoreType configuration:nil URL:storeURL options:options error:&error]) {
        NSLog(@"ERROR: %@", error);
    }
}

- (NSPersistentStoreCoordinator *)persistentStoreCoordinator4
{
    if (_persistentStoreCoordinator != nil) {
        return _persistentStoreCoordinator;
    }
    
    _persistentStoreCoordinator = [[NSPersistentStoreCoordinator alloc] initWithManagedObjectModel:[self managedObjectModel]];
    
    NSDictionary* sourceMetadata = [self sourceMetadata];
    
    NSManagedObjectModel *destinationModel = [_persistentStoreCoordinator managedObjectModel];
    BOOL pscCompatibile = [destinationModel isConfiguration:nil
                                compatibleWithStoreMetadata:sourceMetadata];
    
    if (!pscCompatibile) {
        [self migrateStore];
    }
    
    NSURL *storeURL = [self previousStoreURL];
    NSError* error = nil;
    NSDictionary *options = @{NSMigratePersistentStoresAutomaticallyOption: @YES,
                              NSInferMappingModelAutomaticallyOption: @YES,
                              NSSQLitePragmasOption: @{@"journal_mode": @"WAL"}};
    
    if (![_persistentStoreCoordinator addPersistentStoreWithType:NSSQLiteStoreType configuration:nil URL:storeURL options:options error:&error]) {
        [self removeOldStoreCompletelyAndAddTheNewOne];
    }
    
    return _persistentStoreCoordinator;
}


5) Until now, I haven’t done any bug fixing. All I did was to create an environment that will allow me to analyze the bug easily and fix it. Actually, as I refactored the code, I got a better understanding of the mechanics of the Core Data migration and I got a good idea of where the bug is. So, I was actually kind of ‘bug-fixing’ as I was cleaning up the code; it is a win-win. I have identified the bug; it was located in the moving of the stores in the file directory (no need to go into the details here). Here is how the code looks like with the bug fixed:


  1
  2
  3
  4
  5
  6
  7
  8
  9
 10
 11
 12
 13
 14
 15
 16
 17
 18
 19
 20
 21
 22
 23
 24
 25
 26
 27
 28
 29
 30
 31
 32
 33
 34
 35
 36
 37
 38
 39
 40
 41
 42
 43
 44
 45
 46
 47
 48
 49
 50
 51
 52
 53
 54
 55
 56
 57
 58
 59
 60
 61
 62
 63
 64
 65
 66
 67
 68
 69
 70
 71
 72
 73
 74
 75
 76
 77
 78
 79
 80
 81
 82
 83
 84
 85
 86
 87
 88
 89
 90
 91
 92
 93
 94
 95
 96
 97
 98
 99
100
101
102
103
104
105
106
- (id) appDelegate { return [[UIApplication sharedApplication] delegate]; }

- (NSString*) previousStoreName { return VERSION_1_STORE_NAME; }
- (NSString*) currentStoreName { return  VERSION_2_STORE_NAME; }

- (NSURL*) previousStoreURL {
    NSURL* directoryUrlForStores = [[self appDelegate] applicationCachesDirectory];
    return [directoryUrlForStores URLByAppendingPathComponent:[self previousStoreName]];
}

- (NSURL*) currentStoreURL {
    NSURL* directoryUrlForStores = [[self appDelegate] applicationCachesDirectory];
    return [directoryUrlForStores URLByAppendingPathComponent:[self currentStoreName]];
}

- (BOOL) isThereANeedToSwitchOverToTheNewModel {
    
    NSURL* existingStoreURL = [self previousStoreURL];
    BOOL weHaveAnExistingModel = [[NSFileManager defaultManager] fileExistsAtPath:existingStoreURL.path];
    
    NSURL* newStoreURL = [self currentStoreURL];
    BOOL newModelWasAlreadyCreated = [[NSFileManager defaultManager] fileExistsAtPath:newStoreURL.path];
    
    return weHaveAnExistingModel && !newModelWasAlreadyCreated; // We have existing data and we haven't yet created the new model
}

- (void) migrateToTheNewModelWithError: (NSError**) migrationError {
    
    NSURL* existingStoreURL = [self previousStoreURL];
    NSURL* newStoreURL = [self currentStoreURL];
    
    BOOL success = [self migrateStore:existingStoreURL toVersionTwoStore:newStoreURL error:migrationError];
    
    if (success) {
        DDLogVerbose(@"store converted from %@ to %@", existingStoreURL, newStoreURL);
        [self removeStoreAndItsRelatedFilesForStoreName:[self previousStoreName]];
    }
    else {
        DDLogError(@"Failed store conversion from %@ to %@ due to error:%@", existingStoreURL, newStoreURL, (*migrationError).description);
    }
}

- (BOOL)migrateStore:(NSURL *)storeURL toVersionTwoStore:(NSURL *)dstStoreURL error:(NSError **)outError {

    NSMappingModel *mappingModel = [NSMappingModel inferredMappingModelForSourceModel:[self sourceModel] destinationModel:[self managedObjectModel] error:outError];

    if (!mappingModel) {
        return NO;
    }
    
    // Create a migration manager to perform the migration.
    NSMigrationManager *manager = [[NSMigrationManager alloc] initWithSourceModel:[self sourceModel] destinationModel:[self managedObjectModel]];
    
    NSDictionary *options = @{NSMigratePersistentStoresAutomaticallyOption: [NSNumber numberWithBool:YES], NSInferMappingModelAutomaticallyOption: [NSNumber numberWithBool:YES]};
    BOOL success = [manager migrateStoreFromURL:storeURL
                                           type:NSSQLiteStoreType
                                        options:options withMappingModel:mappingModel
                               toDestinationURL:dstStoreURL
                                destinationType:NSSQLiteStoreType
                             destinationOptions:options error:outError];
    return success;
}

- (void) removeStoreAndItsRelatedFilesForStoreName: (NSString*) storeName {
    
    NSError *contentsError = nil;
    id appDelegate = [[UIApplication sharedApplication] delegate];
    NSArray *directoryContents = [[NSFileManager defaultManager] contentsOfDirectoryAtURL:[appDelegate applicationCachesDirectory]
                                                               includingPropertiesForKeys:@[(NSString *)kCFURLNameKey]
                                                                                  options:NSDirectoryEnumerationSkipsSubdirectoryDescendants |NSDirectoryEnumerationSkipsPackageDescendants | NSDirectoryEnumerationSkipsHiddenFiles
                                                                                    error:&contentsError];
    for (NSURL *fileUrlToDelete in directoryContents) {
        NSString *fileName = [fileUrlToDelete.path lastPathComponent];
        if ([fileName hasPrefix:storeName]) {
            [[NSFileManager defaultManager] removeItemAtURL:fileUrlToDelete error:nil];
            DDLogVerbose(@"File deleted at path: %@", fileUrlToDelete.path);
        }
    }
}

- (NSPersistentStoreCoordinator *)persistentStoreCoordinator
{
    if (_persistentStoreCoordinator != nil) {
        return _persistentStoreCoordinator;
    }
    
    _persistentStoreCoordinator = [[NSPersistentStoreCoordinator alloc] initWithManagedObjectModel:[self managedObjectModel]];
    
    if ([self isThereANeedToSwitchOverToTheNewModel]) {
        
        NSError* migrationError = nil;
        [self migrateToTheNewModelWithError:&migrationError];
        
        if (migrationError) {
            [self removeStoreAndItsRelatedFilesForStoreName:[self currentStoreName]];
        }
    }
    
    
    NSError* persistentStoreError = nil;
    if (![_persistentStoreCoordinator addPersistentStoreWithType:NSSQLiteStoreType configuration:nil URL:[self currentStoreURL] options:nil error:&persistentStoreError]) {
        DDLogVerbose(@"Persistent store wasn't added due to the error: %@", persistentStoreError.description);
    }
    
    return _persistentStoreCoordinator;
}


6) The bug is fixed and the code is refactored. The main method looks a lot cleaner now; the [isThereANeedToSwitchOverToTheNewModel] method lets you know if we need to do migration or not. If migration is required, it is done in the [migrateToTheNewModelWithError:] method. So, if there is a problem, you now have an idea of where to look.

DISCUSSION

 It was a good refactoring example that showed that how we can fix bugs and clean up the code at the same time. These two processes actually do go hand-in-hand.

I think there is some future work left to do:

1) At this point, it might make sense to take out the migration logic to its own class. There are around 100 lines of code and it is responsible solely for the migration. So, it looks like a good candidate for extraction.

2) With regards to refactoring, there is still one issue remaining (probably there is a lot more, but there is one conceptual issue that is bugging me a lot). Take a look at the main method once again.

 1
 2
 3
 4
 5
 6
 7
 8
 9
10
11
12
13
14
15
16
17
18
19
20
21
22
23
24
25
26
- (NSPersistentStoreCoordinator *)persistentStoreCoordinator
{
    if (_persistentStoreCoordinator != nil) {
        return _persistentStoreCoordinator;
    }
    
    _persistentStoreCoordinator = [[NSPersistentStoreCoordinator alloc] initWithManagedObjectModel:[self managedObjectModel]];
    
    if ([self isThereANeedToSwitchOverToTheNewModel]) {
        
        NSError* migrationError = nil;
        [self migrateToTheNewModelWithError:&migrationError];
        
        if (migrationError) {
            [self removeStoreAndItsRelatedFilesForStoreName:[self currentStoreName]];
        }
    }
    
    
    NSError* persistentStoreError = nil;
    if (![_persistentStoreCoordinator addPersistentStoreWithType:NSSQLiteStoreType configuration:nil URL:[self currentStoreURL] options:nil error:&persistentStoreError]) {
        DDLogVerbose(@"Persistent store wasn't added due to the error: %@", persistentStoreError.description);
    }
    
    return _persistentStoreCoordinator;
}

Can you spot the issue here? Don't think about functional problems, it is related to the design of the method. Be the first one to comment with the right answer, and you will receive 5$ in your Paypal account.

Cheers,
Guven.

Comments

  1. AnonymousDecember 15, 2014 at 2:32 AM

    Your passion to refactor and understand code is awesome. It's great to see a kindred spirit :).

    Somethings I do additionally before I change the code:
    - Characterization Tests :). I write unit tests to capture the existing behavior before touching the code. This makes me understand the method contract and I know that I haven't broken something else. Refactoring is so much fun, when you can run tests with a keyboard shortcut every 30 seconds.

    - Comments. Before I touch the code, I write one line comments explaining what code below is supposed to do. This again aids my understanding, the comment is a future name the refactored code below and I think reasoning about the structure of the code is better when your brain is in comments mode rather than detailed code mode( which is similar to the paired programming idea of one person wearing the hat of the implementer and the other wearing the hat of the designer. I think thinking in terms of comments(future method names) aids the designing part.
    - A question about iOS? no try catch blocks?
    - If you want to remove more duplication, there is duplication in previousStoreURL and currentStoreURL. The method names provide you with the hint that there is duplication there as well :)
    - Why is migration happening in the persistentStoreCoordinator? Does it violate the Single Responsibility Principle? Everything from line 9-17 'seems' unrelated to the responsibility of the Singleton Creation Pattern that the persistentStoreCoordinator method seems to be about. Maybe there is a temporal coupling here which need not be?Maybe lines 9 - 17 should be part of a migration routine run when the app is run for the first time?

    It's great to see you put the variables close to the code trick. I remember SAP coding practices suggesting otherwise for better readability :). I look forward to your other articles!

    (Rajiv A.)

    ReplyDelete
  2. AnonymousDecember 15, 2014 at 2:38 AM

    What a fantastic comeback! I've been watching Cosmos these days on Netflix, and in the micro-cosmos that is our profession, I've found your opening very profound: "Almost every software developer maintains code one way or the other. But, does every software developer write maintainable code?". If we're always maintaining code, it comes to reason that we should also write maintainable code!

    In my opinion, maintainable code is code that is easy to change, and that comes from being easy to read and easy to understand. I've found a great post about it these days: https://medium.com/bloc-posts/lets-get-serious-about-readability-4e4ce6a9b6c2

    Your post is great, a pragmatic incursion into our everyday decisions about code evolution. Too bad most professional developers don't think like you do, we wouldn't even discuss maintainability if all code was good code!

    About your method, I think Rajiv is very right! The method name doesn't tell me it's doing a migration and also there's that business with the singleton. But, in a sense, you could argue that that's all internal business of the method and that could be OK if those characteristics don't ever interfere with how the method works for its callers. I don't know enough about Core Data to know that.

    Other things that I can think of:

    - If the migration fails, you delete the current store in method persistentStoreCoordinator, but if the migration succeeds, you delete the previous store in migrateToTheNewModelWithError. Having all methods at the same level of abstraction (SLAP principle) would further improve the code readability.
    - If the call to addPersistentStoreWithType, you still return without any indication of an error, only a log message (maybe that's OK because this is seen as a bug, but are you sure it won't happen on the wild?)
    - Do you need to synchronize access to the method, because of the singleton instance and the migration business?
    - You can allocate and initialize the _persistentStoreCoordinator after the migration business.
    - What happens if you add version 3 of your store?

    (Rodrigo J.)

    ReplyDelete
  3. James GuvenDecember 15, 2014 at 3:20 AM

    Some comments from my side:

    - Tests are indeed missing from this article, thanks Rajiv. As Rodrigo mentioned, maintainable code refers to code that is easily changeable. Readability is very important, but if we can't easily play with the code then it can rot very easily.

    - Rajiv, your comment made me realize another problem with the method design. This method is actually a getter; I feel that doing migration inside a getter is a bit off. So, I would probably take it out elsewhere.

    - Rodrigo, you spotted the problem that I had in mind! "Having all methods at the same level of abstraction (SLAP principle) would further improve the code readability". I think, there are multiple levels of abstraction in this method. At the top, I am abstracting the migration logic but still exposing the physical moving of stores. At the bottom, I am at a very different layer; actually performing framework level calls via [addPersistentStoreWithType: configuration: URL: options: error:]. So, it feels like the method is all over the place when it comes to cognitive load.

    - Perfect article Rodrigo, very relevant here. It also mentions Aspect Oriented Programming, which is always welcome here :)

    So, Rodrigo you won!! Send me over your Paypal account. If you don't feel comfortable, I can do a donation to Wikipedia instead. Up to you :)

    ReplyDelete
  4. JordãoDecember 15, 2014 at 5:24 AM

    Haha,

    It's Wikipedia then.... Not that I feel uncomfortable, but I really think Wikipedia is the best site in the whole world wide web,

    ReplyDelete

Post a Comment

Popular Posts