1
\$\begingroup\$

Wrote my first mini-script to get data from the german football league table and save the data to a csv file. Do you like the approach or could I be more efficient? Thanks in advance!

from bs4 import BeautifulSoup
import requests
import csv

url = "https://www.bundesliga.com/de/bundesliga/tabelle"

r = requests.get(url)

r_soup = BeautifulSoup(r.content, "lxml")

table = r_soup.find_all("tr")

csv_file = open("bundesliga_table.csv", "w")

csv_writer = csv.writer(csv_file)
csv_writer.writerow(["Rank", "Team", "Matches", "Points", "Goal Difference"])

for club in table:
    try:
        rank = club.find("td", class_="rank").text
        team = club.find("span", class_="d-none d-lg-inline").text

        matches = club.find("td", class_="matches").text
        points = club.find("td", class_="pts").text
        difference = club.find("td", class_="difference").text


        print(str(rank) + " " + str(team) + " " + str(matches) + " " + str(points) + " " + str(difference))
        csv_writer.writerow([rank, team, matches, points, difference])

    except:
        print("One team not found")

csv_file.close()
\$\endgroup\$
  • 3
    \$\begingroup\$ The current question title of your question is too generic to be helpful. Please edit to the site standard, which is for the title to simply state the task accomplished by the code. Please see How do I ask a good question?. \$\endgroup\$ – BCdotWEB Apr 2 at 9:49
1
\$\begingroup\$

Great answers, as always. Just a few remarks from me:

There is not enough validation in your project. You are scraping a website that could change at any time, and your script is expecting DOM elements that may not be there. So you need to check each of them.

From the doc (emphasis is mine):

If find_all() can’t find anything, it returns an empty list. If find() can’t find anything, it returns None

To avoid repetition, instead of repeatedly calling club.find (even if it's just a few times), you could have a for loop, using a list or dictionary containing the DOM elements being sought and the matching HTML attribute. Then you validate the existence of the element and extract the text value in the same pass. Thus, your code becomes more solid and easier to maintain. On the other hand, you have just 5 elements in this code. But your next project may involve retrieving a lot more.

Your HTTP request can fail too, for lots of reasons like lost connectivity. Then the rest of your code will fail. I suggest to wrap the HTTP request in its own try/catch block and stop execution if it fails. There is no point trying to parse the HTML if it was not retrieved.

It is good to have exception handling in the functions that do specific tasks, but the main function should also have its own generic exception handler. Advice: log every exception to a file. Especially if the script is going to run unattended.

One last thing: you should always test your code in less than ideal conditions: try to run it against another, arbitrary website, or a domain name that does not even exist, and see how it behaves.

| improve this answer | |
\$\endgroup\$
1
\$\begingroup\$

I'll go out on a limb and say that there is nothing clearly wrong with this script. As a casual batch file, the only improvement I would suggest using the "Main Method" pattern. I've yet to find a clear, focused explanation of both what the pattern is and why one should use it, but this is a good start.

Oh, and while it's good that you're calling csv_file.close(), it's greatly preferred to use with open('filename', 'x') as csv_file: instead.

As for efficiency: There are probably ways you could make this script a little more performant, but for such a simple task it's probably counter-productive. It would be relatively a lot of work, and it would make the script harder to work on, so unless you're scraping huge amounts of data it's probably not worth it.

Within the tools you're already using, one thing that could make this look nicer would be to use csv.DictWriter(), DictWriter.writeheader(), and .writerows(). In order for writerows() to really work well for you, you'll probably want to learn about lists, list comprehensions (and/or map), generators and iterables, and functions. And of course to use DictWriter you'll need to learn about dictionaries. And if you're going to learn about functions, it's a good idea to learn about type hints and type checkers. and so on and so forth!

I wouldn't be here if I could help myself from banging out untested scripts for other people's problems:

from bs4 import BeautifulSoup
import csv
from pprint import pprint
import requests
import sys
from typing import Dict

default_file = "bundesliga_table.csv"
default_url = "https://www.bundesliga.com/de/bundesliga/tabelle"
fields_functions = {
    "Rank": lambda club_tr: club_tr.find("td", class_="rank").text,
    "Team": lambda club_tr: club_tr.find("span", class_="d-none d-lg-inline").text,
    "Matches": lambda club_tr: club_tr.find("td", class_="matches").text,
    "Points": lambda club_tr: club_tr.find("td", class_="pts").text,
    "Goal Difference": lambda club_tr: club_tr.find("td", class_="difference").text
}

def main():
    argc = len(sys.argv)
    file = sys.argv[1] if 1 < argc else default_file
    url = sys.argv[2] if 2 < argc else default_url
    scrape_to_file(file, url)

def scrape_to_file(target_file: str, source_url: str) -> None:
    source = BeautifulSoup(requests.get(source_url).content, "lxml")
    data = source.find_all("tr")
    with open(target_file, "w", newline='') as csv_file:
        csv_writer = csv.DictWriter(csv_file, fields_functions.keys())
        csv_writer.writeheader()
        csv_writer.writerows(parse(club) for club in data)

def parse(club_tr) -> Dict[str, str]:
    try:
        parsed = {key: func(club_tr) for key, func in fields_functions.items()}
        pprint(parsed.values())
        return parsed
    except Exception as e:
        pprint("Error parsing one row!")
        pprint(e)
        return {}

if __name__ == "__main__":
    main()
```
| improve this answer | |
\$\endgroup\$
0
\$\begingroup\$

Two things about the way you handle files:

  1. Since you open the file and only close it at the end, if an exception disrupts your program in between, the file will not be properly closed. Instead use a with context manager:

    with open("bundesliga_table.csv", "w") as csv_file:
        ...
    

    This automatically closes the file for you when leaving the block, whether by having finished the code within or due to an exception.

  2. Currently you are writing one row at a time. However, the writer can also take an iterable of rows and write them all at once. This allows it to potentially optimize the writing in a better way.

The latter also gives you the opportunity to put your parsing code into a function. Actually, you should probably put all your code into functions! This way you can give them nice and clear names, add a docstring describing what the function does and how to use it and add type annotations if you want to. This makes your code much more readable, especially when it grows to more than the few lines you currently have. It also decouples getting the data from displaying it and from writing it to a file, all of which are separate things that are mushed together in your current code. Again, that is fine for small scripts or as a starting point, but when your scripts get larger you want to refactor this.

You should also be careful with try...except clauses. The bare except you currently have will also catch e.g. the user pressing Ctrl+C if they want to abort the process (because it is taking too long or whatever). Using except Exception will avoid that at least, but you should catch as specific as possible.

In this case I would use something like this:

from bs4 import BeautifulSoup
import requests
import csv
from itertools import chain

def get_table(url):
    """Parse the official bundesliga website to get the current table.

    Returns an iterable of rows."""
    r = requests.get(url)
    r.raise_for_status()
    soup = BeautifulSoup(r.content, "lxml")
    for club in soup.find_all("tr"):
        try:
            rank = club.find("td", class_="rank").text
            team = club.find("span", class_="d-none d-lg-inline").text
            matches = club.find("td", class_="matches").text
            points = club.find("td", class_="pts").text
            difference = club.find("td", class_="difference").text
            yield rank, team, matches, points, difference
        except Exception:
            print("Did not find a team:")
            print(club)

def write_csv(rows, file_name):
    """Write an iterable of rows to the CSV file file_name."""
    with open(file_name, "w") as csv_file:
        writer = csv.writer(csv_file)
        writer.writerows(rows)

if __name__ == "__main__":
    url = "https://www.bundesliga.com/de/bundesliga/tabelle"
    rows = list(get_table(url))
    for row in rows:
        print(" ".join(map(str, row)))
    header = ["Rank", "Team", "Matches", "Points", "Goal Difference"]
    write_csv(chain(header, rows), "bundesliga_table.csv")

Note that I used a if __name__ == "__main__": guard to allow importing from this script from another script without the scraping being run. The get_table function returns a generator, which you can just iterate over and it produces the values as it goes. But since we need the content both for printing and for writing, we need to persist it using list. It also has a r.raise_for_status, which will raise an exception if getting the webpage failed for whatever reason, which means you know right away that you are not connected to the internet and not only when it cannot parse the (not-existing) website.

| improve this answer | |
\$\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.