3
\$\begingroup\$

I'm new to Python, and just started to learn the principles of Object Oriented Programming, this does not judge strictly.

This code is fully working but confused some elements:

  1. I don't understand what the __init__ method needs to return.
  2. I don't like self. in get_parse that much.

Initially, I did it in order to be able to easily cause Parse.id, or you want me to attribute.

But now, I need to first create an instance of the class, and then call the function get_parse while accepting self and the number of lines (k), and only then, I will be available for items such as Parse.title.

I tried to do the same when declaring __init__, but nothing came of it, because at this stage the function may not be available (this is especially the language, is not it? In IPython not exist, or am I mistaken too?)

from bs4 import BeautifulSoup
import urllib.request
import csv

class Parse:

    k = 1

    def __init__(self,k):
        pass


    def read_csv(k):
        with open('/home/narnikgamarnik/PycharmProjects/my_phyton3_projects/products_links2.csv') as f:
            r = csv.reader(f)
            cont = [row for row in r]
            d = (cont[k])[0]
            return d


    def get_url(d):
        try:
            url = urllib.request.urlopen(d)
        except urllib.error.HTTPError as err:
            if err.code == 404:
                return False
            else:
                raise
        return url


    def get_title(url):
        try:
            soup = BeautifulSoup(url, 'html.parser')
            ol = soup.find('ol', 'breadcrumb')
            title = ol.find_all('li')[-1].string
        except AttributeError:
            return False
        return title

    def get_gender(url):
        try:
            soup = BeautifulSoup(url, 'html.parser')
            ol = soup.find('ol', 'breadcrumb')
            gender = ol.find_all('a')[0].string
        except AttributeError:
            return False
        return gender

    def get_category(url):
        try:
            soup = BeautifulSoup(url, 'html.parser')
            ol = soup.find('ol', 'breadcrumb')
            brand = ol.find_all('a').get_text[1].string
        except AttributeError:
            return False
        return brand


    def get_model(url):
        try:
            soup = BeautifulSoup(url, 'html.parser')
            ol = soup.find('ol', 'breadcrumb')
            model = ol.find_all('a')[2].string
        except AttributeError:
            return False
        return model


    def get_article(url):
        try:
            soup = BeautifulSoup(url, 'html.parser')
            product_code = soup.find('p', 'product__code')
            article = product_code.find_all('span')[0].string
        except AttributeError:
            return False
        return article


    def get_article_2(url):
        try:
            soup = BeautifulSoup(url, 'html.parser')
            geth1 = soup.find('h1')
            article_2 = geth1.find_all('span')[0].string
        except AttributeError:
            return False
        return article_2


    def get_prices(url):
        try:
            soup = BeautifulSoup(url, 'html.parser')
            product_price = soup.find_all('span', 'select_currency currency hide')
        except AttributeError:
            return False
        return product_price


    def get_img(url):
        try:
            soup = BeautifulSoup(url, 'html.parser')
            div = soup.find_all('div', 'fotorama fotorama-primary')
            for a in div:
                b = a.find_all('a')
                images = []
                for c in b:
                    d = c['data-full']
                    images.append(d)
        except AttributeError:
            return False
        return images


    def get_parse(self,k):
        self.d = self.read_csv(k)
        self.url = self.get_url(self.d)
        self.title = self.get_title(self.url)
        self.url = self.get_url(self.d)
        self.gender = self.get_gender(self.url)
        self.url = self.get_url(self.d)
        self.category = self.get_category(self.url)
        self.url = self.get_url(self.d)
        self.model = self.get_model(self.url)
        self.url = self.get_url(self.d)
        self.article = self.get_article(self.url)
        self.url = self.get_url(self.d)
        self.article_2 = self.get_article_2(self.url)
        self.url = self.get_url(self.d)
        self.prices = self.get_prices(self.url)
        self.price_pln = self.prices[0].string[3:6]
        self.price_usd = self.prices[1].string[3:6]
        self.price_eur = self.prices[2].string[3:6]
        self.price_gbp = self.prices[3].string[3:6]
        self.price_rub = self.prices[4].string[3:7]
        self.url = self.get_url(self.d)
        self.images = self.get_img(self.url)
        return self.d, self.title, self.gender, self.category, self.model, self.article, self.article_2, self.images, self.price_pln, self.price_usd, self.price_eur, self.price_gbp, self.price_rub, self.images

Usage is as follows:

from parse import parse

parse = parse.Parse
k = 1
parse.get_parse(parse,k)

Tell me how to make a code to be beautiful? I'm a novice at programming, but the wording of code seems cumbersome and ugly, although I understand that the syntax of the language, and so makes coding more enjoyable than js

\$\endgroup\$
4
  • \$\begingroup\$ This code does not feel like it could work. For example, the call self.read_csv(k) does not match with the definition of the function def read_csv(k):. Same for pretty much all your "methods". Can you provide the calling code, because I don't see how it could work with only this part? \$\endgroup\$ – 301_Moved_Permanently Sep 7 '16 at 12:15
  • \$\begingroup\$ from parse import parse parse = parse.Parse k = 1 parse.get_parse(parse,k) \$\endgroup\$ – Narnik Gamarnik Sep 7 '16 at 12:41
  • \$\begingroup\$ and this - worldbox.pl/en/… first product link from 'products-link2.csv' \$\endgroup\$ – Narnik Gamarnik Sep 7 '16 at 12:42
  • \$\begingroup\$ Yes, and more. It seems silly to re-declare as many self.url = self.get_url (self.d). Is there a way to get rid of it? \$\endgroup\$ – Narnik Gamarnik Sep 7 '16 at 12:44
5
\$\begingroup\$

Your biggest problem is that you are not using classes properly.

All class methods should have self as the first parameter. __init__ is the function that is called, when an instance of a class is created (which you do not do!). In that case you also don't need to constantly reassign class attributes like self.url.

class Parser:
    def __init__(self, file_name, k):
        self.file_name = file_name
        self.k = k
        self.url = self.get_url(...)
    ...
    def parse(self):
        return self.read_csv()

    def read_csv(self):
        return self.file_name, self.k


parser = Parser('/home/narnikgamarnik/PycharmProjects/my_phyton3_projects/products_links2.csv', 1)
print parser.parse()

Your second biggest problem is redundancy. All your get_* methods do something very similar:

  1. Instantiate a BeatifulSoup object
  2. find all x tags
  3. (Find all y tags)
  4. Return some part of those tags

You could write a function that dynamically does this, and store the date what to use for which category (title, category, gender, ...) in some data structure like a dict of collections.namedtuple:

import collections

Category = collections.namedtuple("Category", "pos element1 element2 method")

class Parse:
    def __init__(self, k):
        self.data = {'title': Category(-1, 'ol', 'li', 'breadcrumb'),
                     'gender': Category(0, 'ol', 'a', 'breadcrumb'),
                     'category': Category(1, 'ol', 'a', 'breadcrumb'),
                     'model': Category(2, 'ol', 'a', 'breadcrumb'),
                     'article': Category(0, 'p', 'span', 'product_code'),
                     'article2': Category(0, 'h1', 'span', '')}

    def parse(self):
        return [self.get(url, *data) for data in self.data]

    def get(self, url, pos, element1, element2, method):
        try:
            soup = BeautifulSoup(url, 'html.parser')
            if method:
                elements = soup.find(element1, method)
            else:
                elements = soup.find(element1)
            return elements.find_all(element2)[pos].string
        except AttributeError:
            return False

The other might actually need separate methods.

\$\endgroup\$
2
  • \$\begingroup\$ I started to think in this direction, but the lack of skill and understanding of OOP did not give me a chance to do better. Your examples seem slozhnyim, but understandable. Thank you so much! On codereview right to make an answer to my question, or to replace my first post? \$\endgroup\$ – Narnik Gamarnik Sep 7 '16 at 15:28
  • \$\begingroup\$ Yeah, they are not complete examples, you still have something to flesh out. But I hope you get the general idea. \$\endgroup\$ – Graipher Sep 7 '16 at 15:31
5
\$\begingroup\$

The first thing that hit me when reading your code is that your class isn't one. It's just a mere collection of functions. Following usual class definition, your get_parse function should not even execute properly. However, as regard to your usage, it can work because the self parameter is actually the class itself.

This is not how it is supposed to work and I encourage you to read a tutorial on classes in Python. As I said, your current code is only a collection of functions, and you could start by letting it as such:

from bs4 import BeautifulSoup
import urllib.request
import csv


def read_csv(k):
    with open('/home/narnikgamarnik/PycharmProjects/my_phyton3_projects/products_links2.csv') as f:
        r = csv.reader(f)
        cont = [row for row in r]
        d = (cont[k])[0]
        return d


def get_url(d):
    try:
        url = urllib.request.urlopen(d)
    except urllib.error.HTTPError as err:
        if err.code == 404:
            return False
        else:
            raise
    return url


def get_title(url):
    try:
        soup = BeautifulSoup(url, 'html.parser')
        ol = soup.find('ol', 'breadcrumb')
        title = ol.find_all('li')[-1].string
    except AttributeError:
        return False
    return title

def get_gender(url):
    try:
        soup = BeautifulSoup(url, 'html.parser')
        ol = soup.find('ol', 'breadcrumb')
        gender = ol.find_all('a')[0].string
    except AttributeError:
        return False
    return gender

def get_category(url):
    try:
        soup = BeautifulSoup(url, 'html.parser')
        ol = soup.find('ol', 'breadcrumb')
        brand = ol.find_all('a').get_text[1].string
    except AttributeError:
        return False
    return brand


def get_model(url):
    try:
        soup = BeautifulSoup(url, 'html.parser')
        ol = soup.find('ol', 'breadcrumb')
        model = ol.find_all('a')[2].string
    except AttributeError:
        return False
    return model


def get_article(url):
    try:
        soup = BeautifulSoup(url, 'html.parser')
        product_code = soup.find('p', 'product__code')
        article = product_code.find_all('span')[0].string
    except AttributeError:
        return False
    return article


def get_article_2(url):
    try:
        soup = BeautifulSoup(url, 'html.parser')
        geth1 = soup.find('h1')
        article_2 = geth1.find_all('span')[0].string
    except AttributeError:
        return False
    return article_2


def get_prices(url):
    try:
        soup = BeautifulSoup(url, 'html.parser')
        product_price = soup.find_all('span', 'select_currency currency hide')
    except AttributeError:
        return False
    return product_price


def get_img(url):
    try:
        soup = BeautifulSoup(url, 'html.parser')
        div = soup.find_all('div', 'fotorama fotorama-primary')
        for a in div:
            b = a.find_all('a')
            images = []
            for c in b:
                d = c['data-full']
                images.append(d)
    except AttributeError:
        return False
    return images


def get_parse(k):
    d = read_csv(k)
    url = get_url(d)
    title = get_title(url)
    url = get_url(d)
    gender = get_gender(url)
    url = get_url(d)
    category = get_category(url)
    url = get_url(d)
    model = get_model(url)
    url = get_url(d)
    article = get_article(url)
    url = get_url(d)
    article_2 = get_article_2(url)
    url = get_url(d)
    prices = get_prices(url)
    price_pln = prices[0].string[3:6]
    price_usd = prices[1].string[3:6]
    price_eur = prices[2].string[3:6]
    price_gbp = prices[3].string[3:6]
    price_rub = prices[4].string[3:7]
    url = get_url(d)
    images = get_img(url)
    return d, title, gender, category, model, article, article_2, images, price_pln, price_usd, price_eur, price_gbp, price_rub, images

Usage being:

from parse import parse

parse.get_parse(1)

Next, you call get_url several times with the same data d as parameter. This is useless as you already have computed the value once, you could just reuse that:

def get_parse(k):
    d = read_csv(k)
    url = get_url(d)
    title = get_title(url)
    gender = get_gender(url)
    category = get_category(url)
    model = get_model(url)
    article = get_article(url)
    article_2 = get_article_2(url)
    prices = get_prices(url)
    price_pln = prices[0].string[3:6]
    price_usd = prices[1].string[3:6]
    price_eur = prices[2].string[3:6]
    price_gbp = prices[3].string[3:6]
    price_rub = prices[4].string[3:7]
    images = get_img(url)
    return d, title, gender, category, model, article, article_2, images, price_pln, price_usd, price_eur, price_gbp, price_rub, images

As a side note, your namming is a bit wrong: the URL is what is return from read_csv and it is passed as a parameter to "get_url". What you get from get_url is an HTML document. And that document is parsed using BeautifulSoup in each get_XXX function. You might want to do the job only once here too:

def get_html_soup(url):
    response = urllib.request.urlopen()
    return BeautifulSoup(response, 'html.parser')

def get_title(soup):
    ol = soup.find('ol', 'breadcrumb')
    try:
        title = ol.find_all('li')[-1].string
    except AttributeError:
        return False
    return title

def get_gender(soup):
    ol = soup.find('ol', 'breadcrumb')
    try:
        gender = ol.find_all('a')[0].string
    except AttributeError:
        return False
    return gender

def get_category(soup):
    # You get it

...

def get_parse(k):
    url = read_csv(k)
    soup = get_html_soup(url)
    title = get_title(soup)
    gender = get_gender(soup)
    category = get_category(soup)
    model = get_model(soup)
    article = get_article(soup)
    article_2 = get_article_2(soup)
    prices = get_prices(soup)
    price_pln = prices[0].string[3:6]
    price_usd = prices[1].string[3:6]
    price_eur = prices[2].string[3:6]
    price_gbp = prices[3].string[3:6]
    price_rub = prices[4].string[3:7]
    images = get_img(soup)
    return d, title, gender, category, model, article, article_2, images, price_pln, price_usd, price_eur, price_gbp, price_rub, images

Note that I removed the error handling on the download part: if an error occured while downloading the URL, you want to know why; and if the download succeded, parsing the HTML should work just fine.


Now looking at the use case, I would define get_parse as accepting an URL directly and using it like:

def get_parse(url):
    soup = get_html_soup(url)
    title = get_title(soup)
    gender = get_gender(soup)
    category = get_category(soup)
    model = get_model(soup)
    article = get_article(soup)
    article_2 = get_article_2(soup)
    prices = get_prices(soup)
    price_pln = prices[0].string[3:6]
    price_usd = prices[1].string[3:6]
    price_eur = prices[2].string[3:6]
    price_gbp = prices[3].string[3:6]
    price_rub = prices[4].string[3:7]
    images = get_img(soup)
    return d, title, gender, category, model, article, article_2, images, price_pln, price_usd, price_eur, price_gbp, price_rub, images

if __name__ == '__main__':
    url = read_csv(1)
    print(get_parse(url))

This let's you compute the URLs first and then process them all in a for loop if you need be without having to reload the CSV file each time.

You could even change your read_csv to yield URLs one by one such as:

def read_csv(filename='/home/narnikgamarnik/PycharmProjects/my_phyton3_projects/products_links2.csv'):
    with open(filename) as f:
        r = csv.reader(f)
        for row in r:
            yield row[0]


def get_parse(url):
    soup = get_html_soup(url)
    title = get_title(soup)
    gender = get_gender(soup)
    category = get_category(soup)
    model = get_model(soup)
    article = get_article(soup)
    article_2 = get_article_2(soup)
    prices = get_prices(soup)
    price_pln = prices[0].string[3:6]
    price_usd = prices[1].string[3:6]
    price_eur = prices[2].string[3:6]
    price_gbp = prices[3].string[3:6]
    price_rub = prices[4].string[3:7]
    images = get_img(soup)
    return d, title, gender, category, model, article, article_2, images, price_pln, price_usd, price_eur, price_gbp, price_rub, images


if __name__ == '__main__':
    for url in read_csv():
        print('URL', url, 'got me:')
        print(get_parse(url))

Note that I also change the way the filename is defined. So you could easily call read_csv('/path/to/another/file') if need be.

\$\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.