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

Better support for count #12

Closed
ornicar opened this issue Apr 14, 2011 · 6 comments
Closed

Better support for count #12

ornicar opened this issue Apr 14, 2011 · 6 comments

Comments

@ornicar
Copy link
Contributor

ornicar commented Apr 14, 2011

Actually we have Type::getCount but it does not accept a query. However, elasticsearch supports it (http://www.elasticsearch.org/guide/reference/api/count.html).

So I'm about to fix that, by adding Searchable::count($query)

@ruflin
Copy link
Owner

ruflin commented Apr 14, 2011

Is it necessary that every searchable object also has to support count? Like this every time I use searchable for an object, I also have to implement count.

Refactoring/enhancing of the function is a good idea. It's probably Elastica_Type::count(Elastica_Query_Abstract $query = null)

If the query is not set, a match all query should be created.

@ornicar
Copy link
Contributor Author

ornicar commented Apr 14, 2011

"Is it necessary that every searchable object also has to support count?"
Well as elasticsearch provides count each time search is available, I'd say yes...
We can also keep these features separated by adding a Countable interface, but I feel it makes less sense, especially when you want to paginate search results: the count is needed as much as the search.
Elasticsearch keeps count and search very similar and I think Elastica should reflect this consistency. By giving both methods the same signature, ($query).
So, move the query construction logic to a new protected method to avoid code duplication.
Thoughts?

@ruflin
Copy link
Owner

ruflin commented Apr 14, 2011

I somehow like the idea of splitting up searchable and countable, as I did not use count yet and would like to keep it separate. But it is also ok for me to "merge" it at the moment. Interesting is that PHP also has a countable interface (without parameters): http://php.net/manual/en/class.countable.php

To be honest, I do not like my implementation of the query processing inside the search function, especially because the same processing has to be done for every search function. I already thought about introducing a search object that would be passed to the search function. But I'm not sure if this would make everything more complicated because I like to just pass a string to the search function. Or the other option could be to use the search/count object inside the search function. But I have to think more about this first.

So I think at the moment it's best to move it to a protected method.

@ornicar
Copy link
Contributor Author

ornicar commented Apr 14, 2011

There is indeed a minor flaw in the architecture. The searchable classes are given too much responsibilities. An easy way to reduce the code duplication between searchable classes is to move the query construction to a static method Elastica_Query::create. That's what I am doing now.

@ornicar
Copy link
Contributor Author

ornicar commented Apr 14, 2011

-> #13

@ornicar ornicar closed this as completed Apr 14, 2011
@ruflin
Copy link
Owner

ruflin commented Apr 14, 2011

Good idea. I do not really like that we have to use a static function for this one, but at the moment it looks like the best solution.

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

No branches or pull requests

2 participants