4
\$\begingroup\$

I've done this parser to scrape all footwear data, but I don't know if it is good to use OOP in this case. Can you please check this out and give me the strongest feedback? I'm working on improving my coding style and looking for an opportunity to do coding better. How can I make this parser more maintainable for the future?

#!/usr/bin/env python3
# coding=utf-8

import re
from datetime import datetime
from bs4 import BeautifulSoup
import http
import requests
import urllib.request


html_hdrs = {'User-Agent': 'Mozilla/5.0 (X11; Linux x86_64) AppleWebKit/537.11 ('
         'KHTML, like Gecko) Chrome/23.0.1271.64 Safari/537.11',
         'Accept': 'text/html,application/xhtml+xml,application/xml;q=0.9,*/*;q=0.8',
         'Accept-Charset': 'ISO-8859-1,utf-8;q=0.7,*;q=0.3',
         'Accept-Encoding': 'none',
         'Accept-Language': 'en-US,en;q=0.8',
         'X-Requested-With': 'XMLHttpRequest',
         'Connection': 'keep-alive'}


def make_soup(input_url):
    try:
        req = urllib.request.Request(input_url, headers=html_hdrs)
        html = urllib.request.urlopen(req).read()
    except http.client.IncompleteRead as e:
        html = e.partial
    return BeautifulSoup(html, "html.parser")



class Items(object):
    """Exporting all Matches Fashion website items
    to a list of soups"""

    def __init__(self):
        self._ul_tag = 'ul'
        self._li_tag = 'li'
        self._href_attr = 'href'
        self.main_url = 'https://www.matchesfashion.com'
        self._gender_urls = ('https://www.matchesfashion.com/intl/mens/shop/shoes',
                             'https://www.matchesfashion.com/intl/womens/shop/shoes',
                             'https://www.matchesfashion.com/intl/mens/sale/shoes',
                             'https://www.matchesfashion.com/intl/womens/sale/shoes')

    def _get_categories(self):
        category_box_class = 'filter__box__list'
        sale_category_box_class = 'innerFilterMobile catLevel3'
        out_categories = set()
        for gender in self._gender_urls:
            if '/sale' in gender:
                raw_page = make_soup(gender)
                raw_categories = raw_page.findAll(name=self._ul_tag, class_=sale_category_box_class)
                raw_categories = raw_categories[0].findAll(name=self._li_tag)
                for raw_category in raw_categories:
                    out_categories.add('{}{}'.format(self.main_url, raw_category.a[self._href_attr]))
            else:
                raw_page = make_soup(gender)
                raw_categories = raw_page.findAll(name=self._ul_tag, class_=category_box_class)
                raw_categories = raw_categories[0].findAll(name=self._li_tag)
                for raw_category in raw_categories:
                    out_categories.add('{}{}'.format(self.main_url, raw_category.a[self._href_attr]))
        self._category_urls = out_categories

    def _get_pages(self):
        pagination_class = 'redefine__right__pager'
        self._pages = list()

        def _paginate(input_url, input_num):
            out_url = '{}?page={}&noOfRecordsPerPage=240&sort='.format(input_url, input_num)
            return out_url

        for url in self._category_urls:
            page_soup = make_soup(url)
            last_page_num = page_soup.findAll(name=self._ul_tag, class_=pagination_class)
            if len(last_page_num) != 0:
                last_page_num = int(last_page_num[0].findAll(name=self._li_tag)[-3].text)
                for page_num in range(1, last_page_num+1):
                    page_list_url = _paginate(url, page_num)
                    self._pages.append(page_list_url)
            else:
                self._pages.append(url)

    def get(self):
        def get_gender(input_url):
            if '/womens/' in input_url:
                return 'women'
            elif '/mens/' in input_url:
                return 'men'

        def is_sale(input_url):
            if '/sale' in input_url:
                return True
            else:
                return False

        def get_category(input_url):
            if '?page=' in input_url:
                out_category = re.search(r'/shoes/(.*)(\?)', input_url).group(1)
            else:
                out_category = re.search(r'/shoes/(.*)', input_url).group(1)
            return out_category
        out_items = list()
        item_class = 'lister__item'
        self._get_categories()
        self._get_pages()
        for url in self._pages:
            gender = get_gender(url)
            category = get_category(url)
            sale = is_sale(url)
            url_soup = make_soup(url)
            url_soup = url_soup.findAll(name=self._li_tag, class_=item_class)
            for item_soup in url_soup:
                out_items.append((sale, gender, category, item_soup))
        return out_items


class Info(object):
    """Getting item info out of soup to an Info object"""

    def __init__(self, input_item):
        self._div_tag = 'div'
        self._strike_tag = 'strike'
        self._span_tag = 'span'
        self._li_tag = 'li'
        self._alt_attr = 'alt'
        self._pic_attr = 'data-original'
        self._href_attr = 'href'
        self._shop_name = 'Matches Fashion'
        self.sale, self.gender, self.category, self.item_soup = input_item

    def _get_raw_info(self):
        vendor_class = 'lister__item__title'
        self.title = self.item_soup.img[self._alt_attr]
        self.description = self.title
        self.model = self.title
        self.time = datetime.now(timezone('Europe/Moscow')).strftime('%Y-%m-%d %H:%M:%S')
        self.picture = 'https:{}'.format(self.item_soup.img[self._pic_attr])
        self.url = 'https://www.matchesfashion.com{}'.format(self.item_soup.a[self._href_attr])
        self.vendor = self.item_soup.findAll(name=self._div_tag, class_=vendor_class)[0].text
        self.shop = self._shop_name
        self.freeship = False

    def _get_prices(self):
        full_price_class = 'lister__item__price-full'
        new_price_class = 'lister__item__price-down'
        discount_class = 'lister__item__price-save'
        try:
            self.price = self.item_soup.findAll(name=self._span_tag, class_=full_price_class)[0].text
            self.oldprice = self.price
            self.discount = 0
        except IndexError:
            self.price = self.item_soup.findAll(name=self._span_tag, class_=new_price_class)[0].text
            self.oldprice = self.item_soup.findAll(name=self._strike_tag)[0].text
            self.discount = self.item_soup.findAll(name=self._span_tag, class_=discount_class)[0].text

    def _get_sizes(self):
        no_stock_class = 'noStock'
        raw_sizes = self.item_soup.select("{}[class!={}]".format(self._li_tag, no_stock_class))
        self.sizes = []
        for raw_size in raw_sizes:
            size = raw_size.text
            self.sizes.append(size)
        if len(raw_sizes) != 0:
            size = raw_sizes[0].text
            if 'eu' in size.lower():
                self.sizetype = 'eu'
            if 'us' in size.lower():
                self.sizetype = 'us'
            if 'uk' in size.lower():
                self.sizetype = 'uk'

    def get(self):
        self._get_raw_info()
        self._get_prices()
        self._get_sizes()


raw_items = Items()
items = raw_items.get()

for item in items:
    info = Info(item)
    info.get()

This code is a bit raw, but the only thing that is left is the one more class Process(object) to process all data left.

\$\endgroup\$
  • \$\begingroup\$ The current question title, which states your concerns about the code, is too general to be useful here. Please edit to the site standard, which is for the title to simply state the task accomplished by the code. Please see How to get the best value out of Code Review: Asking Questions for guidance on writing good question titles. \$\endgroup\$ – BCdotWEB Jul 30 at 12:47
  • \$\begingroup\$ @BCdotWEB Thanks! I've tried to change the title, but I'm more interested in a feedback and advises of how to make this code better to be readable and to maintain. \$\endgroup\$ – sshkrv Jul 30 at 12:52
  • \$\begingroup\$ Such expectations belong in the body of your question, not your title. As I state in my comment: "the site standard, which is for the title to simply state the task accomplished by the code". \$\endgroup\$ – BCdotWEB Jul 30 at 12:53
  • \$\begingroup\$ Welcome to Code Review! What Python version did you write this for and why doesn't it seem like this code ever exports its data? \$\endgroup\$ – Mast Jul 30 at 13:02
  • 1
    \$\begingroup\$ @Mast Thank you! it's Python 3.6 and it doesn't exports the data just because I haven't written the Process class yet. The data has to be processed before export. But the last lines of code creates objects with data inside. \$\endgroup\$ – sshkrv Jul 30 at 13:09
1
\$\begingroup\$

It looks very procedural at a glance, it might just not be useful to use custom objects here. In any case you don't need to explicitly inherit from object, class Process: is fine already.

So, more maintainable, well, the headers look dubious, but obviously chosen from a real-life browser. The character set / encoding values would be the most concerning to me wrt. maintenance, but likely the encoding detection will take care of that. Similarly the Connection probably doesn't belong there and should be handled by the HTTP client instead (like most of them, except the user agent I suppose).

_ul_tag, _li_tag, _href_attr those are way unlikely to change. If defined at all they should be global constants instead.

The _gender_urls should be constructed from the main_url.

_get_categories has two almost identical blocks, better unify them into a new method.

I'd perhaps consider move all of the named classes etc. that might change into constants at the start of the class or file so changing some doesn't require having to look all over the file. Then again, minor complaint and perhaps not worth it if they're really only used once. Giving them good names is nice.

_paginate can just return '...'.format(...) instead of having the variable? Goes for some more places in the code too. If the variable is only used once and the line is short enough, just inline them.

get_gender - what if neither is in the input?

is_sale can simply be return '/sale' in input_url.

get_category can be simplified for sure. Also the regex can be precompiled with compile once and globally instead of every time the method runs. Also the two branches can be simplified into a single regex, look for "non-greedy" matching in the re documentation.

_get_sizes - the append bit could be a for comprehension, self.sizes = [raw_size.text for raw_size in raw_sizes]. For most (all?) objects if len(x) != 0 will mean exactly the same as if x:, though that might be a stylistic thing. Hmm and the whole if ... in size.lower(): part, well firstly, what about if none of them match? Or more than one? Actually I'd suggest a loop here, lower = size.lower(); for x in ['eu', 'us', 'uk']: if x in lower: self.sizetype = x.

You probably also want if __name__ == '__main__': ... at the bottom instead of just running code whenever the file is loaded.

\$\endgroup\$
  • 1
    \$\begingroup\$ These are really useful to me, thanks! By the way, what do you think about using inheritance to create more parsers with other shops? I mean, what if I create new parser with such OOP style, but is it good to inherit these classes? Or it's better to create just the same ones? Or maybe it's a good idea to use OOP at all? \$\endgroup\$ – sshkrv Aug 23 at 12:45
  • 1
    \$\begingroup\$ Right, well I can see that being useful if there's common functionality between different parsers. Do you think that will be the case for other shops? I imagine the structure might be too different. I'd first do one more then see if you can share code on the whole class level and if not extract helper functions etc. as you notice duplication or patterns occurring. \$\endgroup\$ – ferada Aug 23 at 15:49

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.