2
\$\begingroup\$

This code works but is very slow. I am scraping a website for urls for various books, and then checking to see if we have complete records of those books in our database. After I process the URL to get the book info, I see if we have it in our database. If we don't, I move on, that's fine. If we do, I need to check that we have a matching BookTextVer type; that foreign keys to the book, and filters on an attribute from the URL. If we have it, great; if not, I add it to my list of dictionaries to eventually pass to the django template.

def book_missing(request):
    data = []
    books = book.objects.all()
    sitemap = "www.sitemap.com"
    page = requests.get(sitemap)
    tree = objectify.fromstring(page.content)
    regex = re.compile("books-(\d+)([A-Za-z]+)(\d+)([A-Za-z]+)")
    for node in tree.getchildren():
        book_url = node.book.text
        # e.g. https://www.sitemap.com/fdsys/pkg/books-27hr200nf/content-detail.html
        m = regex.search(book_url)
        object_type_cd = m.groups()[1]  # hr
        book_num = m.groups()[2]  # 200
        book_type = m.groups()[3]  # nf
        try:
            book = books.get(book_num=book_num,
                             object_type_cd_id=object_type_cd.upper())
        except book.DoesNotExist:
            pass
        if (BookTextVer.objects.filter(
            book=book,
            book_text_ver_type__short_name=book_type.upper()).exists()):
            pass  # matching book type exists, no processing needed
        else:
            data.append({"book": object_type_cd.upper() + book_num,
                         "book_type": book_type.upper()})
    context = {
        'data': data,
    }
    return render(request, 'etl/reports/gpo-book-missing.html', context)
\$\endgroup\$
2
\$\begingroup\$

After I process the URL to get the book info, I see if we have it in our database. If we don't, I move on, that's fine.

I guess you're referring to this part of the code:

try:
    book = books.get(book_num=book_num,
                     object_type_cd_id=object_type_cd.upper())
except book.DoesNotExist:
    pass

This doesn't do what you described, because after the pass, the rest of the loop body is executed instead of moving on the next iteration. Perhaps you meant to use continue there instead of pass.


Instead of having an empty if block and an else block here, it would be better to flip the condition and have only an if block without an else:

if (BookTextVer.objects.filter(
    book=book,
    book_text_ver_type__short_name=book_type.upper()).exists()):
    pass  # matching book type exists, no processing needed
else:
    data.append({"book": object_type_cd.upper() + book_num,
                 "book_type": book_type.upper()})

The query on BookTextVer tries to match by short_name. Is that field indexed in the database. If not, the query will be inevitably slow, and indexing should be a significant improvement.


The current implementation runs 1 or 2 queries for every node in tree.getchildren(). You might get better performance if you first collect all the book_num, book_type, object_type_cd values from the document, and formulate a single database query or constant number of database queries. That would be a major change, and possibly you might need to forego Django's OR-mapping benefits, but the reduced number of database queries could make a big difference.

\$\endgroup\$
  • \$\begingroup\$ You're right on the pass vs continue... I don't know why that didn't trigger an error since I use that (non-existent) book in the next part of the query \$\endgroup\$ – thumbtackthief Jun 22 '17 at 20:41

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.