views:

121

answers:

4

I have an entity called 'Job' with two boolean attributes named 'completed' and 'logged'. I am trying to retrieve all completed jobs that have not been logged at app start-up and change them to logged. I'm able to get all the completed but unlogged jobs with this fetchRequest:

NSPredicate *predicate = [NSPredicate predicateWithFormat:@"(completed == %@ && logged == %@)", [NSNumber numberWithBool:YES], [NSNumber numberWithBool:NO]];

I'm then assigning this predicate to a fetchRequest and calling the [managedObjectContext executeFetchRequest:fetchRequest] method to get an array of all Job entities that meet this criteria. This seems to work fine and is returning the correct number of jobs.

What I've been trying to do is loop through the NSArray returned, set the logged attribute to YES and then save. This seems to complete and doesn't return any errors but the changes are not persisted when the application quits. Where am I going wrong?

[fetchRequest setPredicate:predicate];
NSError error;
NSArray jobsToLog = [managedObjectContext executeFetchRequest:fetchRequest error:&error];
if ([jobsToLog count] > 0) {
   for (int i = 0; i < [jobsToLog count] - 1; i++) {
      [[jobsToLog objectAtIndex:i] setLogged:[NSNumber numberWithBool:YES]];
      // Commit the changes made to disk
      error = nil;
      if (![managedObjectContext save:&error]) {
         // An error occurred
      }
   }
}

Thanks in anticipation,

A: 

What if you use the KVC (Key Value Coding) method:

[[jobsToLog objectAtIndex:i] setValue:[NSNumber numberWithBool:YES] forKey:@"logged"];

Do you have a setLogged method in your custom NSManagedObject class?

Diederik Hoogenboom
A: 

I'm no expert here but I think you need to tell the managed object context by using setPrimitiveValue: to make the changes. So try this.

[self willChangeValueForKey:@"logged"];
[self setPrimitiveValue:[NSNumber numberWithBool:YES] forKey:@"logged"];
[self didChangeValueForKey:@"logged"];
regulus6633
+1  A: 

Some notes that may help:

The for loop is looping from 0 to one less than the count. This will skip the last job. If there is only 1 job, nothing will happen. Change the loop to:

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

or

for (int i = 0; i <= [jobsToLog count] - 1; i++)

When pulling objects out of NSArray the compiler will not know its type. You should explicitly cast the object:

  [(Job *)[jobsToLog objectAtIndex:i] setLogged:[NSNumber numberWithBool:YES]];

You can do both of the above using fast enumeration:

for(Job *thisJob in jobsToLog) {
  [thisJob setLogged:[NSNumber numberWithBool:YES];
}
MrHen
Casting is completely unnecessary because you are getting an id back which will accept any method call as long as make the class aware that the method exists via imports. Casting in Objective-C is wrong 99.9% of the time.
Marcus S. Zarra
@Marcus S. Zarra: Can you explain your comment for me? I have been pondering it for awhile and don't fully understand why using id is better than explicitly casting what you are expecting in the array.
MrHen
+1  A: 

First, you can clean things up a bit via:

[fetchRequest setPredicate:predicate];
NSError *error = nil;
NSArray jobsToLog = [managedObjectContext executeFetchRequest:fetchRequest error:&error];
NSAssert1(error == nil, @"Error retrieving jobs: %@", [error userInfo]);
for (id job in jobsToLog) {
  [job setValue:[NSNumber numberWithBool:YES] forKey:@"logged"];
}
if (![managedObjectContext save:&error]) {
  NSAssert1(NO, @"Failed to save %@", [error userInfo]);
}
  • Note that I start with error being nil and being a pointer (your declaration was not). If you do not then the initial state of that pointer is undefined and definitely not nil.
  • Check to make sure there is no error in the fetch.
  • The check against count is unnecessary as the fast enumerator will handle it for you.
  • Saving after every object is wasteful, unless you have thousands of objects, save at the end.
  • Walk through your code in the debugger and make sure you are getting objects back and are looping over them.
Marcus S. Zarra
Thanks very much! Much cleaner code than I had churned out :)
Garry