4
\$\begingroup\$

I have created a BeautifulSoup vacancies parser, which works, but I do not like how it looks. Therefore, I'd be very happy if somebody could give me any improvements.

from urllib.request import urlopen
from bs4 import BeautifulSoup

page = urlopen('http://rabota.ua/zapros/python/%D0%BA%D0%B8%D0%B5%D0%B2')
soup = BeautifulSoup(page, "lxml")
vacancies = {}

a = soup.find_all("a", class_="t")
inx = 0
for x in a:
    inx += 1
    vacancies[inx] = {'position': x.contents[0].strip(),
                      'link': 'http://rabota.ua{}'.format(x.get('href'))}

a = soup.find_all("a", class_="rua-p-c-default")[2:]
inx = 0
for x in a:
    inx += 1
    vacancies[inx].update({'company': x.get_text()})

for x in vacancies:
    print('{}\n{}\n{}\n'.format(vacancies[x]['company'], vacancies[x]['position'], vacancies[x]['link']))

Here is the HTML code:

enter image description here

\$\endgroup\$
  • 3
    \$\begingroup\$ Welcome to Code Review! As we all want to make our code more efficient or improve it in one way or another, try to write a title that summarizes what your code does, not what you want to get out of a review. Please see How to get the best value out of Code Review - Asking Questions for guidance on writing good question titles. \$\endgroup\$ – 409_Conflict Nov 7 '16 at 13:02
  • 2
    \$\begingroup\$ Apart from what @MathiasEttinger suggested, I'd also ask for a description about what this code is trying to do. \$\endgroup\$ – Grajdeanu Alex. Nov 7 '16 at 13:06
  • 2
    \$\begingroup\$ A screenshot isn't ideal. Please edit your post to replace it with the actual HTML markup. \$\endgroup\$ – Mathieu Guindon Nov 7 '16 at 15:03
  • 2
    \$\begingroup\$ @Mat'sMug True, but in some ways a prettified view of the DOM tree is preferable to a dump of nasty-looking HTML. \$\endgroup\$ – 200_success Nov 7 '16 at 18:19
11
\$\begingroup\$

A couple of suggestions:

  1. Use requests instead of urllib. This is mostly a matter of preference but I think you'll find it easier to work with.

An example on how this would look is:

import requests

page = requests.get('http://rabota.ua/zapros/python/%D0%BA%D0%B8%D0%B5%D0%B2').content
  1. Fix the bug

In some cases you'll get:

Traceback (most recent call last):
http://rabota.ua/company209907/vacancy6312501 vacancies[x]['link'])) UnicodeEncodeError: 'ascii' codec can't encode characters in position 26-33: ordinal not in range(128)

Which means you're trying to parse characters which cannot be decoded as they are. I'd recommend using the encode function to get rid of it:

for x in vacancies:
    print('{}\n{}\n{}\n'.format(
        vacancies[x]['company'].encode('ascii', 'ignore'),
        vacancies[x]['position'].encode('ascii', 'ignore'),
        vacancies[x]['link'].encode('ascii', 'ignore')))

Instead of using a counter, you could use the builtin that Python already has: enumerate().

This:

inx = 0
for x in a:
    inx += 1
    vacancies[inx].update({'company': x.get_text()})

Will become:

for counter, x in enumerate(a, 1):
    vacancies[counter].update({'company': x.get_text()})

So far, we have this:

from bs4 import BeautifulSoup
import requests

page = requests.get('http://rabota.ua/zapros/python/%D0%BA%D0%B8%D0%B5%D0%B2').content
soup = BeautifulSoup(page, "lxml")

a = soup.find_all("a", class_="t")

vacancies = {}
for counter, x in enumerate(a, 1):
    vacancies[counter] = {
        'position': x.contents[0].strip(),
        'link': 'http://rabota.ua{}'.format(x.get('href'))
    }

a = soup.find_all("a", class_="rua-p-c-default")[2:]

for counter, x in enumerate(a, 1):
    vacancies[counter].update({'company': x.get_text()})

for x in vacancies:
    print('{}\n{}\n{}\n'.format(
        vacancies[x]['company'].encode('ascii', 'ignore'),
        vacancies[x]['position'].encode('ascii', 'ignore'),
        vacancies[x]['link'].encode('ascii', 'ignore')))

Other suggestions:

I'm not a fan of your naming conventions:

  • a might become href_tag
  • x might become element

More, I'd split everything into functions as this will make your code readable and easier to maintain:

from bs4 import BeautifulSoup
import requests

URL = 'http://rabota.ua/zapros/python/%D0%BA%D0%B8%D0%B5%D0%B2'


def get_html():
    return BeautifulSoup(requests.get(URL).content, 'lxml')


def parse_html():
    content = get_html()
    href_tag = content.find_all('a', class_='t')

    vacancies = {}
    for counter, element in enumerate(href_tag, 1):
        vacancies[counter] = {
            'position': element.contents[0].strip(),
            'link': 'http://rabota.ua{}'.format(element.get('href'))
        }

    href_tag = content.find_all("a", class_="rua-p-c-default")[2:]

    for counter, element in enumerate(href_tag, 1):
        vacancies[counter].update({'company': element.get_text()})

    for element in vacancies:
        print('{}\n{}\n{}\n'.format(
            vacancies[element]['company'].encode('ascii', 'ignore'),
            vacancies[element]['position'].encode('ascii', 'ignore'),
            vacancies[element]['link'].encode('ascii', 'ignore'))
        )


if __name__ == '__main__':
    parse_html()

Yet another way of splitting the logic of your program:

from bs4 import BeautifulSoup
import requests

URL = 'http://rabota.ua/zapros/python/%D0%BA%D0%B8%D0%B5%D0%B2'


def get_html():
    return BeautifulSoup(requests.get(URL).content, 'lxml')


def get_position_and_link(content):
    href_tag = content.find_all('a', class_='t')

    vacancies = {}
    for counter, element in enumerate(href_tag, 1):
        vacancies[counter] = {
            'position': element.contents[0].strip(),
            'link': 'http://rabota.ua{}'.format(element.get('href'))
        }

    return vacancies


def update_info(content):
    company_href_tag = content.find_all("a", class_="rua-p-c-default")[2:]
    vacancies = get_position_and_link(content)

    for counter, element in enumerate(company_href_tag, 1):
        vacancies[counter].update({'company': element.get_text()})
    return vacancies


def main():
    content = get_html()
    vacancies = update_info(content)

    for element in vacancies:
        print('{}\n{}\n{}\n'.format(
            vacancies[element]['company'].encode('ascii', 'ignore'),
            vacancies[element]['position'].encode('ascii', 'ignore'),
            vacancies[element]['link'].encode('ascii', 'ignore'))
        )

if __name__ == '__main__':
    main()

You can see that I also added if __name__ == '__main__'. By doing the main check, you can have that code only execute when you want to run the module as a program and not have it execute when someone just wants to import your module and call your functions themselves.

As @Mathias suggested in comments, instead of using the encode() function, you might as well do:

def main():
    content = get_html()
    vacancies = update_info(content)

    for element in vacancies:
        for info in ('company', 'position', 'link'):
            print(vacancies[element][info])
        print('\n')

The advantage of this method might appear when you have a lot of unrecognised ascii characters which, with the first version, may lead to something like: .,..

\$\endgroup\$
  • \$\begingroup\$ This is a very helpful answer, thank you! I will study it and correct mistakes. \$\endgroup\$ – Jrh Nov 7 '16 at 15:05
  • \$\begingroup\$ Why not suggest for info in ('company', 'position', 'link'): print(vacancies[element][info]) instead of using encode? \$\endgroup\$ – 409_Conflict Nov 7 '16 at 15:54
  • \$\begingroup\$ @MathiasEttinger I just wanted to avoid the nested for loop ^_^ But if I think of it, it might return better results so I'll add it just for OPs' info. Thanks for head-ups \$\endgroup\$ – Grajdeanu Alex. Nov 7 '16 at 16:18
6
\$\begingroup\$

Building on @Dex'ter answer, I think that using a dictionary to store your results call for the wrong iteration patterns. I would store them in a list and grab them directly with a for loop when I need them.

Building becomes something along the lines of:

vacancies = [{
    'position': element.contents[0].strip(),
    'link': 'http://rabota.ua{}'.format(element.get('href')),
} for element in href_tag]

Updating would be:

for vacancy, company in zip(vacancies, company_href):
    vacancy.update({'company': company.get_text()})

And printing can be as simple as:

for vacancy in vacancies:
    print(vacancy['company'], vacancy['position'], vacancy['link'], sep='\n')

Writting that, I think that using the whole building + updating technique is also a poor decision as you could build everything at once:

href_tag = content.find_all('a', class_='t')
company_href_tag = content.find_all("a", class_="rua-p-c-default")[2:]

vacancies = [{
    'position': element.contents[0].strip(),
    'link': 'http://rabota.ua{}'.format(element.get('href')),
    'company': company.get_text(),
} for element, company in zip(href_tag, company_href_tag)]
\$\endgroup\$
  • \$\begingroup\$ Thank you for answer, I was just thinking how to get rid of the double cycle. I also thought how to do only one find_all instead of two, for example content.find_all("a", class_="rua-p-c-default t") is this real? \$\endgroup\$ – Jrh Nov 7 '16 at 19:10
  • 1
    \$\begingroup\$ @Vadim Reading the documentation, I think content.find_all('a', class_=['t', 'rua-p-c-default']) should work. But you will get a single list as output containing both classes (and probably interleaved too). \$\endgroup\$ – 409_Conflict Nov 7 '16 at 20:03

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.