5
\$\begingroup\$

How would I go about shortening this code? I'm sure there's a smarter way to write the code. It's working fine, it's just ugly.

It is a way to scrape the number of items on an e-commerce site.

import requests
from bs4 import BeautifulSoup
import re

url1 = "https://us.pandora.net/en/charms/?sz=30&start={}&format=page-element"
url2 = "https://us.pandora.net/en/bracelets/?sz=30&start={}&format=page-element"
url3 = "https://us.pandora.net/en/rings/?sz=30&start={}&format=page-element"
url4 = "https://us.pandora.net/en/necklaces/?sz=30&start={}&format=page-element"
url5 = "https://us.pandora.net/en/earrings/?sz=30&start={}&format=page-element"

#res = requests.get(link.format(url1),headers={"User-Agent":"Mozilla/5.0"})

soup1 = BeautifulSoup(requests.get(url1.format(0)).text, 'lxml')
soup2 = BeautifulSoup(requests.get(url2.format(0)).text, 'lxml')
soup3 = BeautifulSoup(requests.get(url3.format(0)).text, 'lxml')
soup4 = BeautifulSoup(requests.get(url4.format(0)).text, 'lxml')
soup5 = BeautifulSoup(requests.get(url5.format(0)).text, 'lxml')

total_items1 = ''.join(re.findall(r'\d', soup1.select_one('span.products-count').text))
total_items2 = ''.join(re.findall(r'\d', soup2.select_one('span.products-count').text))
total_items3 = ''.join(re.findall(r'\d', soup3.select_one('span.products-count').text))
total_items4 = ''.join(re.findall(r'\d', soup4.select_one('span.products-count').text))
total_items5 = ''.join(re.findall(r'\d', soup5.select_one('span.products-count').text))

#categories = [tag['title'].strip() for tag in soup.select('.refinement-link[title]')

#total_items_sale1 = ''.join(re.findall(r'\d', soup1.select_one('.grid-tile .price-standard')))
#total_items_sale1
#total_items_sale1
#total_items_sale1
#total_items_sale1

#print('Categories:')

#for category in categories:
    #print('\t{}'.format(category))
print('\nTotal Charms: {}'.format(total_items1))
print('\nTotal Bracelets: {}'.format(total_items2))
print('\nTotal Rings: {}'.format(total_items3))
print('\nTotal Necklaces: {}'.format(total_items4))
print('\nTotal Earrings: {}'.format(total_items5))
\$\endgroup\$
  • \$\begingroup\$ I'm not sure I spot what you've changed, it still looks the same to me. Could you be more specific? \$\endgroup\$ – doomdaam Jul 5 at 20:24
  • \$\begingroup\$ I added python-3.x and added a final newline. The newline is to make sure that the closing code fences are not shown in the question. \$\endgroup\$ – AlexV Jul 5 at 20:41
  • \$\begingroup\$ Oh, I see. Thanks, AlexV. \$\endgroup\$ – doomdaam Jul 5 at 20:44
6
\$\begingroup\$

If you find yourself, doing something over and over again, loops are often the answer.

Your code rewritten with a loop:

import re

import requests
from bs4 import BeautifulSoup

urls = {
    "Charms": "https://us.pandora.net/en/charms/?sz=30&start={}&format=page-element",
    "Bracelets": "https://us.pandora.net/en/bracelets/?sz=30&start={}&format=page-element",
    "Rings": "https://us.pandora.net/en/rings/?sz=30&start={}&format=page-element",
    "Necklaces": "https://us.pandora.net/en/necklaces/?sz=30&start={}&format=page-element",
    "Earrings": "https://us.pandora.net/en/earrings/?sz=30&start={}&format=page-element"
}

for category, url in urls.items():
    soup = BeautifulSoup(requests.get(url.format(0)).text, 'lxml')
    total_items = ''.join(re.findall(r'\d', soup.select_one('span.products-count').text))
    print("Total {}: {}".format(category, total_items))

So what has changed? The URLs went into a dictionary where the category's name is the key. A simple list would also work here in your case. Now we can iterate over that dictionary. With for category, url in urls.items(): you get the key, i.e. the name of the category and the URL at the same time.

The loop body now does what you had written out by hand, namely getting the page, parse the content with BeautifulSoup and than again with a regex.

Since the URL came bundled with the category, we can no print all the results nicely without writing the same code over and over again.

You can also go a step further and make this into a function:

import re

import requests
from bs4 import BeautifulSoup


def get_total_items(url):
    """Parse the product count from the given pandora URL"""
    soup = BeautifulSoup(requests.get(url).text, 'lxml')
    return ''.join(re.findall(r'\d', soup.select_one('span.products-count').text))


if __name__ == "__main__":
    urls = {
        "Charms": "https://us.pandora.net/en/charms/?sz=30&start={}&format=page-element",
        "Bracelets": "https://us.pandora.net/en/bracelets/?sz=30&start={}&format=page-element",
        "Rings": "https://us.pandora.net/en/rings/?sz=30&start={}&format=page-element",
        "Necklaces": "https://us.pandora.net/en/necklaces/?sz=30&start={}&format=page-element",
        "Earrings": "https://us.pandora.net/en/earrings/?sz=30&start={}&format=page-element"
    }

    for category, url in urls.items():
        print("Total {}: {}".format(category, get_total_items(url.format(0))))

Now the intermediate variable soup is inside the function and does not clutter the global namespace. You could also now easily change the implementation of get_total_items(...) without the need to adapt the "application" code.

I also added the infamous if __name__ == "__main__":, which is basically Python's way to tell someone looking at the script: "This piece of code is supposed to be run if you use this as a script." Have a look at the official documentation or this Stack Overflow post if you want to learn more.


A more subtle change: I reorderd the imports to follow the recommendation of the official Style Guide for Python Code (often just PEP8), which is to have imports from the standard libraries first, then a blank line, and then all the third party libraries.

In general PEP8 is often worth a read, especially if you are not yet familiar with Python.

\$\endgroup\$
  • \$\begingroup\$ Thank you so much!! It has even increased in pace, that's lovely! This is my first time using Stackoverflow and I gotta say, I'm surprised by the good of the community. Never expected people to be so kind, and generous - it is in the end their own time and energy they spend on other peoples code. I also learn alot by looking at the fixes, and different suggestions. Thanks again. \$\endgroup\$ – doomdaam Jul 5 at 20:43
  • \$\begingroup\$ @doomdaam: If you stick to the rules, Code Review is generally a very friendly place where you often can get excellent feedback on your code. It has to be fully functional to the best of your knowledge though. If it's not yet working, Stack Overflow is usually more appropriate. \$\endgroup\$ – AlexV Jul 5 at 20:53
  • \$\begingroup\$ I'm not an expert when it comes to requests. So that might be a better question for Stack Overflow. From what I know, if you really want to send a user agent header, at least use a real one. \$\endgroup\$ – AlexV Jul 5 at 21:13
  • \$\begingroup\$ I figured out! Thanks. \$\endgroup\$ – doomdaam Jul 5 at 21:19
  • \$\begingroup\$ I have a new question regarding the second part of my scraper. I tried to play around with your solution and somehow ues the same mindset of dicts and lists, but without any luck. Here's the thread: [link]codereview.stackexchange.com/questions/223588/… \$\endgroup\$ – doomdaam Jul 5 at 21:46
2
\$\begingroup\$

Starting from the answer by @AlexV, you can reduce the code even more by recognizing that the category name is the only difference in the URLs:

import re
import requests
from bs4 import BeautifulSoup


def get_total_items(url):
    """Parse the product count from the given pandora URL"""
    soup = BeautifulSoup(requests.get(url).text, 'lxml')
    return ''.join(re.findall(r'\d', soup.select_one('span.products-count').text))


if __name__ == "__main__":
    url_template = "https://us.pandora.net/en/{}/?sz=30&start={}&format=page-element"
    categories = "charms", "bracelets", "rings", "necklaces", "earrings"

    for category in categories:
        url = url_template.format(category, 0)
        print(f"Total {category.title()}: {get_total_items(url)}")

I used an f-string (Python 3.6+) to make the string formatting in the for loop a bit more concise and str.title to change the lowercase category name to titlecase.


What you can do after this is use the fact that the requests module can build the request parameters for you. This allows you to also check multiple pages:

import re
import requests
from bs4 import BeautifulSoup

def count_items(response):
    """Parse the product count from the given response"""
    soup = BeautifulSoup(response.text, 'lxml')
    return ''.join(re.findall(r'\d', soup.select_one('span.products-count').text))


def get_total_items(url):
    """Get the items 30 at a time, up to 1000"""
    params = {"sz": 30, "format": "page-element"}
    return sum(count_items(requests.get(url, params=params))
               for params["start"] in range(0, 1000, 30))

if __name__ == "__main__":
    url = "https://us.pandora.net/en/{}/"
    categories = "charms", "bracelets", "rings", "necklaces", "earrings"

    for category in categories:
        total_items = get_total_items(url.format(category))
        print(f"Total {category.title()}: {total_items}")    

This has the step and maximum page number hardcoded, but you could make it an argument of the function. You could also use a requests.Session to reuse the connection to the server, which speeds it up a bit.

\$\endgroup\$
  • \$\begingroup\$ this works like a charm, thank you for your effort! Do you think you could take a look at this code as well? codereview.stackexchange.com/questions/223588/… I'm having a hard time shortening it. \$\endgroup\$ – doomdaam Jul 6 at 17:38
  • \$\begingroup\$ @doomdaam I added some possible next steps, which might be similar to what you want to do \$\endgroup\$ – Graipher Jul 6 at 21:46
  • \$\begingroup\$ very appreciated! Thank you. \$\endgroup\$ – doomdaam Jul 7 at 6:48

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.