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(),

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:


Here is the HTML code:

• 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. – 301_Moved_Permanently Nov 7 '16 at 13:02
• Apart from what @MathiasEttinger suggested, I'd also ask for a description about what this code is trying to do. – Grajdeanu Alex Nov 7 '16 at 13:06
• A screenshot isn't ideal. Please edit your post to replace it with the actual HTML markup. – Mathieu Guindon Nov 7 '16 at 15:03
• @Mat'sMug True, but in some ways a prettified view of the DOM tree is preferable to a dump of nasty-looking HTML. – 200_success Nov 7 '16 at 18:19

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'),


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(),
}

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'),


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(),
}

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'),
)

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')

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

vacancies = {}
for counter, element in enumerate(href_tag, 1):
vacancies[counter] = {
'position': element.contents[0].strip(),
}

return vacancies

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

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'),
)

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: .,..

• This is a very helpful answer, thank you! I will study it and correct mistakes. – Jrh Nov 7 '16 at 15:05
• Why not suggest for info in ('company', 'position', 'link'): print(vacancies[element][info]) instead of using encode? – 301_Moved_Permanently Nov 7 '16 at 15:54
• @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 – Grajdeanu Alex Nov 7 '16 at 16:18

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(),
} 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(),

• @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). – 301_Moved_Permanently Nov 7 '16 at 20:03