jwjacobson / jazztunes

a jazz repertoire management app
https://jazztunes.org
GNU General Public License v3.0
3 stars 1 forks source link

please refactor view to de-duplicate the search query code in if/else #111

Closed bbelderbos closed 8 months ago

bbelderbos commented 8 months ago
          yes two things here:

_Originally posted by @bbelderbos in https://github.com/jwjacobson/jazz_repertoire/pull/101#discussion_r1432887170_

jwjacobson commented 8 months ago

ok I will definitely watch the n+1 video as it seems to keep coming up--

Interestingly no matter how many search terms I give the debug toolbar says each search is only 3 queries

bbelderbos commented 8 months ago

Because search terms is a limited amount?

for term in search_terms[1:]:
    term_query = tunes.filter(...

That's then not that bad actually, but ideally we try to make one query give you can & several Qs.

jwjacobson commented 8 months ago

I am working on this, but a couple further points:

  1. It actually seems to run 3 queries even when there is only one search term, which is odd.
  2. On the other hand, I wonder if the low number is due to this idea that query sets are lazy:

Though this looks like three database hits, in fact it hits the database only once, at the last line (print(q)). In general, the results of a QuerySet aren’t fetched from the database until you “ask” for them. When you do, the QuerySet is evaluated by accessing the database. For more details on exactly when evaluation takes place, see When QuerySets are evaluated.

Haven't read that last link yet but will look once I've gotten things cleaned up a bit

jwjacobson commented 8 months ago

Okay, so the three queries I mentioned (learned this from your n+1 video) are actually just one tune query and two django/allauth queries.

Even when you pass four search terms to the basic search it still runs it in only one query.

jwjacobson commented 8 months ago

I think I'm going to hold off on changing this search tonight and we can just talk about it tomorrow. Definitely in terms of readability and elegance the code needs a rework but it seems that functionally it's already pretty efficient? Thanks to lazy query sets!

jwjacobson commented 8 months ago

Also worth mentioning that I didn't know I was taking advantage of lazy query sets at the time, I thought I was writing the inefficient thing of making three separate queries and then combining them

bbelderbos commented 8 months ago

Good find and sorry for not checking/thinking about the standard auth_user + django_session query that happen so your code indeed was just making one query - great!

bbelderbos commented 8 months ago

So yes you did it in an efficient manner and it's nice that Django has us do the right thing intuitively :)

Only refactoring left is removing the if / else in favor of a single query code block, but that's less urgent right now (I updated the title of this issue).

bbelderbos commented 8 months ago

Done today: https://github.com/jwjacobson/jazz_repertoire/pull/114/files

Great find btw! https://docs.djangoproject.com/en/5.0/ref/models/querysets/#intersection