3
\$\begingroup\$

I wrote a web scraper to get football scores from here. I'm getting the data for all seasons for the three major German leagues. It all works at the moment, but I'm sure it's possible to make it a lot more concise.

import requests
import os
import pandas as pd
from bs4 import BeautifulSoup

# base url for scrape
base_url = "http://www.weltfussball.de/"

ligen = [["Liga1", "bundesliga-2017-2018/"],
         ["Liga2", "2-bundesliga-2017-2018/"],
         ["Liga3", "3-liga-2017-2018/"]]

def get_page(ext):
  # get the page and make soup
  global soup, page
  if (ext[0] == "/"):
    ext = ext[1:]
  url = base_url + ext
  r = requests.get(url)
  page = r.content.decode('utf-8','ignore')
  soup = BeautifulSoup(page, "lxml")

def create_dir(s):
  saison = "saison-" + s.replace("/", "-")
  os.mkdir("data/%s" % saison)

def get_seasons(liga):
  global seasons_df
  # Getting all seasons
  form = soup.find("select", attrs={"name": "saison"})
  seasons = []
  season = []
  for s in form.find_all("option"):
    season.append(s.text)
    season.append(s.get("value"))
    seasons.append(season)
    season = []
  seasons_df = pd.DataFrame(seasons, columns=["Saison", "Link"])
  seasons_df["Spiele"] = "empty"
  seasons_df["Liga"] = str(liga)

def get_games(season_n):
  tables = []
  for t in soup.find_all("table", "standard_tabelle"):
    print(t.contents)
    tables.append(t)

  data_table = tables[0]

  record = []
  records = []
  trs = data_table.findAll("tr")
  for n in range(len(trs)):
    if (trs[n].contents[1].name == "th"):
      th = trs[n].find("th")
      record.append(th.text)
    else:
      if not record:
        record.append(records[-1][0])
      tds = trs[n].findAll("td")
      for p in range(len(tds)):
        record.append(tds[p].text)
      # filling empty date column with previous value
      if (record[1] == ""):
        record[1] = records[-1][1]
      records.append(record)
      record = []

  # Pasting it into a DataFrame
  df = pd.DataFrame(records)

  # Dropping empty columns
  spdf = pd.DataFrame()
  z = 0
  for key in df.keys():
    if (df[key].all() in (" - ", "\n", "\n\n")):
      print("Column %s is empty" % key)
    else:
      print("Adding %s to game DF" % key)
      spdf[z] = df[key]
      z += 1

  # Renaming the columns
  # Defining the new column names
  names = {
      0 : "Spieltag",
      1 : "Datum",
      2 : "Zeit",
      3 : "Heim",
      4 : "Auswärts",
      5 : "Score"
  }

  spdf.rename(columns=names, inplace=True)

  seasons_df["Spiele"][season_n] = spdf

# the execution starts here
for l in range(len(ligen)):
  get_page("alle_spiele/%s" % ligen[l][1])
  get_seasons(ligen[l][0])
  # Getting the data
  for i in seasons_df.index:
    if (ligen[l][0] == "Liga1"):
      try:
        create_dir(seasons_df["Saison"][i])
      except FileExistsError:
        print("Directory %s exists already" % seasons_df["Saison"][i])
      else:
        create_dir(seasons_df["Saison"][i])
    get_page(seasons_df["Link"][i])
    get_games(i)
  ligen[l].append(seasons_df)

for l in range(len(ligen)):
  l1 = ligen[l][2]
  for s in range(len(l1["Saison"])):
    sss = l1["Saison"][s].replace("/", "-")
    sss = sss[0:9]
    l1["Spiele"][s].to_csv("data/saison-%s/%s_Spiele.csv" % (sss, ligen[l][0]), sep=";")

For example, I have to get all tables with class standard_tabelle, because there are two in the page that have no otherwise distinctive attribute.

Data Table:

<div class="data">
  <table class="standard_tabelle" cellpadding="3" cellspacing="1">
    <tbody>
      ...
    </tbody>
  </table>
</div>

Table 2:

<table class="standard_tabelle" cellpadding="3" cellspacing="1">
  <tr>
    <td align="right"><b><a href="/news/2-bundesliga/1/">Aktuelle Meldungen &raquo;</a></b>
    </td>
  </tr>
</table>

I would like to select the data table based on its parent <div class="data">, but there are other divs with the data class, so I can't just find div with class "data".

\$\endgroup\$
2
\$\begingroup\$

Major Issues

Here are some major things that require attention:

  • indentation (PEP8 reference). Use 4 spaces for indentation
  • avoid using global variables. (Why are global variables evil?). If you need to share variables between functions of your program, you can pass them as arguments, or define these functions as method of a "scraper" class which would share instance variables
  • variable naming. The code is much more often read than written - by choosing descriptive variable names, you are improving the readability of your program. Variables like s, ext, t, z, l, l1 etc were bad choices

Next things to address

Web-scraping improvements

  • since you are sending requests to the same domain multiple times, you can improve on the performance by using session.get() instead of a requests.get() where session is initialized as session = requests.Session() once and re-used (reference)
  • you can use SoupStrainer to let BeautifulSoup HTML-parse only the relevant part of the page
  • think of using CSS selectors instead of find() and find_all(). For instance, soup.find("select", attrs={"name": "saison"}) can become soup.select_one("select[name=saison]"). It is not necessarily a better way to locate elements but it is quite handy to have it in your toolbox
  • have you tried to use pandas.read_html() to parse the table HTML block in the get_games() function? It might work to get the table in a dataframe(s) directly

Some simplified code constructs

  • here is a more concise and readable way to define seasons in the get_seasons() function using a list comprehension:

    seasons = [
        [season.get_text(), season.get("value")]
        for season in soup.select("select[name=saison] option")
    ]
    

Overall, though, I think it would be a good idea for this code to go through several rounds of code reviews.

Also, consider using a linting tool like flake8 or pylint, or/and a modern smart IDE like PyCharm - would help to catch a ot of stylistic and other mistakes.

\$\endgroup\$
  • \$\begingroup\$ Thanks for all your comments @alecxe. I have revised the code, what is the best way to upload a revised version? Should I just edit my post and replace the original code? \$\endgroup\$ – iuvbio Nov 26 '17 at 14:44
  • \$\begingroup\$ To some of your points: - I intentionally use 2 spaces intentionally. I can't see any advantage to using 4, it just takes up more space, unless there's something I'm missing. - pd.read_html() works, but the problem is that the headers just get pasted as normal rows then. Instead, I want them as the first column. If there's a way to do that this is certainly better. - Your last suggestion doesn't work, I think because the name attribute has to be specifically passed as an attribute. \$\endgroup\$ – iuvbio Nov 26 '17 at 14:51
  • \$\begingroup\$ @iuvbio ah, right, had a broken code there, should have used select() instead find_all(), fixed, thanks for pointing that out! About 2 spaces, absolutely, certainly your choice, no problem with that, just pointed out what is commonly used. Thanks. \$\endgroup\$ – alecxe Nov 26 '17 at 15:32
  • \$\begingroup\$ Great, that also helped me in selecting the data table right away instead of finding all tables with class standard_tabelle by using data_table = soup.select("div[class=data] table[class=standard_tabelle]"). Do you know how to best add a revised version? If I just replace the code many of your comments won't make sense anymore, but if I add it the post would become very large. \$\endgroup\$ – iuvbio Nov 26 '17 at 15:54
  • 1
    \$\begingroup\$ @iuvbio You can always ask a new question with the revised code. Updating this question would invalidate the answer and would therefore be rolled back. Make sure you link to this first review round, this way reviewers can see what has already been mentioned or changed. \$\endgroup\$ – Graipher Nov 26 '17 at 17:52

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.