3
\$\begingroup\$

Django's get_or_create function returns 2 values: the object and a boolean whether it was freshly created or not. I'm only really interested in the object it returns and this is how I'm doing it.

keywords = [element for (element, _) in [Keyword.objects.get_or_create(keyword=key) for key in keywords]]

But this doesn't look particularly elegant. Is there a better way to do this, possibly avoiding the double loop?

\$\endgroup\$
  • \$\begingroup\$ Maybe a wrapper function to just return the object part? \$\endgroup\$ – User319 Jan 26 '17 at 2:24
8
\$\begingroup\$

I'd say this is a good example on how to not use DB.

The problem here is that for each keyword you will call a DB instead of perform this as a single call, so if you list of keywords is 1000 items then you will do 1000 calls to DB.

You can avoid this by splitting your call to:

1. Get existing keywords

existing_keywords = Keyword.objects.filter(keyword__in=keywords).values_list('keyword', flat=True)

2. Get list of not existing keywords

keywords = set(keywords)
not_existing_keywords = keywords.difference(existing_keywords)

3. Create objects with no existing keywords

Keyword.objects.bulk_create(Keyword(keyword=key) for key in keyword))

4. Now finally query your data from DB

keywords_qs = Keyword.objects.filter(keyword__in=keywords)

Note: If you are running on django 1.10+ and using Postgres then step 4 is not needed, since bulk_create will populate your keyword objects with PKs, otherwise you should do step 4

\$\endgroup\$
  • \$\begingroup\$ That's a really good point! I was so fixated on doing things one way that I completely overlooked the downside of my approach. Thank you, I came looking for one answer but I found a much better one! \$\endgroup\$ – Stupid.Fat.Cat Jan 26 '17 at 16:23
2
\$\begingroup\$

There is no benefit to using unpacking here. Instead just index the resulting object. Just like we used to do before we had unpacking:

# Old way
>>> response = ({...}, False)
>>> obj = response[0]
>>> created = response[1]

# New way
>>> (obj, created) = ({...}, False)

However, as you're writing a list comprehension, you can't use tuple unpacking, in a single comprehension, and so it's better if you just use the old way:

keywords = [Keyword.objects.get_or_create(keyword=key)[0] for key in keywords]

To note, I don't see that large a benefit to this. Rather than removing data here, you can just unpack and discard in the consumer. This will mean if you ever need to know if the object were created you can still retrieve this data.

\$\endgroup\$

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service, privacy policy and cookie policy

Not the answer you're looking for? Browse other questions tagged or ask your own question.