Core Data code for manually creating a lot of entries from old ones

Question

Does this looks ok to everyone?

NSFetchRequest *oldFetchRequest = [[NSFetchRequest alloc] init];
NSEntityDescription *oldEntryEntity = [NSEntityDescription entityForName:@"Entry"
 inManagedObjectContext:oldContext];
[oldFetchRequest setEntity:oldEntryEntity];
int numberOfEntries = [oldContext countForFetchRequest:oldFetchRequest error:nil];
int batchSize = 10;
[oldFetchRequest setFetchBatchSize:10];
int offset = 0;
while (numberOfEntries - offset > 0) {
 [oldFetchRequest setFetchOffset:offset];
 NSError *error;
 NSArray *entries = [oldContext executeFetchRequest:oldFetchRequest error:&error];
 for (NSManagedObject *entry in entries) {
 Entry *newEntry = [NSEntityDescription insertNewObjectForEntityForName:@"Entry"
 inManagedObjectContext:newContext];
 [newEntry setupCreationDate:[entry valueForKey:@"creationDate"] withSave:NO];
 newEntry.entryID = [entry valueForKey:@"entryID"];
 NSMutableOrderedSet *newMediaSet = [[NSMutableOrderedSet alloc] init];
 NSOrderedSet *mediaSet = [entry valueForKey:@"media"];
 int i = 0;
 for (NSManagedObject *media in mediaSet) {
 Media *newMedia = [NSEntityDescription insertNewObjectForEntityForName:@"Media"
 inManagedObjectContext:newContext];
 newMedia.positionInEntry = [NSNumber numberWithDouble:i + 1]; //Potentially needs changing
 newMedia.mediaID = [Entry generateString];
 MediaImageData *imageData = [NSEntityDescription insertNewObjectForEntityForName:@"MediaImageData"
 inManagedObjectContext:newContext];
 if ([newMedia.type isEqualToString:@"Image"]) {
 imageData.data = [media valueForKey:@"originalImage"];;
 }
 else if ([newMedia.type isEqualToString:@"Movie"]) {
 NSURL *movieURL = [NSURL URLWithString:newMedia.movie];
 MPMoviePlayerController *moviePlayer = [[MPMoviePlayerController alloc] initWithContentURL:movieURL];
 [moviePlayer stop];
 UIImage *screenshot = [moviePlayer thumbnailImageAtTime:0.0 timeOption:MPMovieTimeOptionNearestKeyFrame];
 [moviePlayer stop];
 imageData.data = UIImageJPEGRepresentation(screenshot, 1.0);
 }
 newMedia.imageData = imageData;
 newMedia.entry = newEntry;
 [newMediaSet addObject:newMedia];
 i++;
 }
 newEntry.media = newMediaSet;
 }
 [newContext save:&error];
 offset = offset + batchSize;
}

Answer 1

The first big problem I see with this code is here:

int i = 0;
for (NSManagedObject *media in mediaSet) {
 // stuff
 newMedia.positionInEntry = [NSNumber numberWithDouble:i + 1];
 // more stuff
 i++;
}

First of all, if positionInEntry is a property intended to hold some sort of index or something for your Media class, then it should be of the same type that collections tend to use for their indexing: NSUInteger. Although... the object itself probably doesn't need to know what position it is in the array, and as this could change, it's probably best to just not have this property at all.

Next, i is defined as an int, and 1 is an int literal, so we're creating the NSNumber in the wrong way. We should be doing this (if we're going to continue to keep this positionInEntry property):

newMedia.positionInEntry = [NSNumber numberWithInt:i + 1];

Finally, we really shouldn't be doing this in this manner at all. If we want to know what index we're working with, we should use a traditional for loop.

for (int i = 0; i < [mediaSet count]; ++i) {
 NSManagedObject *media = mediaSet[i];
 // stuff
 newMedia.positionInEntry = @(i+1);
 // more stuff
}

However, there's a slightly better option.

It is good to use a forin loop where we can, because this actually is faster than a traditional for loop in Objective-C. We still don't want to rely on i however, for a couple of reasons, right now, I'll say, the most important of which is readability. But there is this option:

for (NSManagedObject *media in mediaSet) {
 // stuff
 NSUInteger index = [mediaSet indexOfObject:media];
 newMedia.positionInEntry = @(index + 1);
 // more stuff
}

We can call indexOfObject: on an NSArray and it will return the first index it finds that object at. As a note, this method returns NSNotFound if the object isn't found in the array, but in a forin loop, this should hopefully never be the case.