Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add count() method to entities and optimize Core Data implementation #87

Merged
merged 2 commits into from
Dec 19, 2014

Conversation

twobitlabs
Copy link

Although the SugarRecord wiki lists a count() method under "Objects querying", there is no such method--you have to call all().count().

In Core Data, this is inefficient, as it loads all instances of the entity and then counts them. I've added a count() method to SugarRecordObjectProtocol as is currently documented, so it can be more conveniently called directly from an object type (e.g., SomeObjectClass.count()).

Also optimized the Core Data implementation of count() to use NSManagedObjectContext.countForFetchRequest() which invokes a count(*) query on the data store instead of loading the entities and counting them.

… directly from an object type (e.g., SomeObjectClass.count()), rather than via all().count().

Optimized core data implementation of count() to use `NSManagedObjectContext.countForFetchRequest()` instead of loading all instances of the object into memory and counting them.
@pepicrft
Copy link

Hello @twobitlabs firstly thanks for your contribution. I agree with the performance fix I didn't think about it when I did the implementation. I've been reviewing it and thinking about the current library structure and I would change it a bit (but keeping the core idea):

  1. You've added a count() method directly to the object protocol. That way we can do something like Person.count(). Internally it creates a finder with the all filter and calls count() on the finder. 👍 here
  2. However when you connect the finder with the context count you're only passing the object class. Remember that the finder includes NSPredicates so the count operation executed on the context with a given class should pass the predicates too and use those predicates internally on the count query.
var count: Int = 0
SugarRecord.operation(stackType!, closure: { (context) -> () in
  count = context.count(self.objectClass!, predicate: self.predicate)
 })
return count

That way the user could do something like:

Person.by("name", value: "Pedro").count()

And the count method will use the internally generated NSPredicate to count People with Pedro as name. What do you think about it?
The rest is perfect for me, do you think we should add any note to the documentation with that?

🎉 🎉 Thanks for all

@twobitlabs
Copy link
Author

Ah nice catch on the predicates. I'll make that change and push. I agree it's a good idea to add this example to the documentation, as I'm not sure it's obvious. Is the wiki the right place for that?

@twobitlabs
Copy link
Author

One thing I found slightly strange was that to create a predicate on a string field, I had to single-quote the string. The Predicate Programming Guide examples show that string matches need to be single-quoted. I think we may need to create a few versions of equalTo that handle different argument types appropriately, or perhaps create different comparators like equalToNumber, likeString, etc. I'll think about it and work on another PR.

@pepicrft
Copy link

Totally agree @twobitlabs . I've created an issue for that: #88

@pepicrft
Copy link

@pepicrft
Copy link

And 👍 to your last changes, got the idea quickly. I'm refactoring a bit the fetching methods to return a custom type other than an Array so I'll update it with these changes and support Realm with better performance. Thanks for all
Merging in 3...2....1

@pepicrft pepicrft added this to the 1.0.4 milestone Dec 19, 2014
pepicrft pushed a commit that referenced this pull request Dec 19, 2014
Add count() method to entities and optimize Core Data implementation
@pepicrft pepicrft merged commit fdcf004 into modo-studio:develop Dec 19, 2014
@pepicrft pepicrft removed the unknown label Dec 19, 2014
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants