Kann bitte jemand über meinen Python-Crawler schauen?

[ford]

Hey,

ich habe einen Crawler geschrieben. Dieser funktioniert auch, jedoch bin ich ziemlich neu in Python und wäre froh, wenn mir jemand Verbesserungsvorschläge geben könnte.


Hier ist der Crawler: https://github.com/ford42/CrawlerHeise


Vielen Dank
LG ford

[sls]

Sollen wir dabei auch auf die Issues / Code-Reviews direkt auf Github eingehen, die teilweise ja auch nicht so ganz richtig sind? Ich kenne von Python nur `requirements.txt` welche idealerweise mit pip freeze erstellt wird. while True: if -> break ist valides Python und sogar die bevorzugte Variante, warum, tonnenweise hier im Forum zu lesen.

Bevor man sich das anschaut, hat das einen Grund dass du IDE-relevante Dateien (.idea) in dein Repo eincheckst? Da solltest du mal lieber dein .gitignore pflegen, das wollen andere bei einem git clone vielleicht nicht mitziehen

[ford]

Danke.

Würdest du den Code wieder auf ed7a162 machen?

Hat keinen Grund, das hat pycharm so gemacht und ich dachte, dass es sich was dabei denkt.

[nezzcarth]

'CrawledArticle' könntest du durch ein 'named tuple' oder auch ein normales Dictionary ersetzen, da die Klasse sich ja nur Daten kapselt und keine Methoden besitzt.

[ford]

#ty, Done

[Sirius3]

Konstanten schreibt man groß: also URL statt url. Wenn CrawledArticle NamedTuple wären, wäre das writerow auch deutlich einfacher.

Eine Klasse pro Datei ist in Python etwas übertrieben. Also könnte man auch alles in __init__.py schreiben, und weil das dann die einzige Datei im Paket wäre, könnte man es zu einem Modul machen.

In ArticleFetcher wird url_parsed als Attribut definiert aber nie benutzt; das kann eine lokale Variable sein; oder man kann es gleich weglassen, denn ob da ›seite=1‹ in der Url steht oder nicht, ist auch egal.

In ›next_page‹ und ›fetch‹ wird jeweils die Seite gelesen und geparst. Das ist einmal zu viel. ›next_page‹ erhält außerdem als Parameter url, was aber immer self.url ist, also ist der Parameter überflüssig. ›next_page‹ würde auch suggerieren, dass die nächste Seite gelesen wird, aber es wird nur geprüft, ob es eine nächste Seite gibt, ›has_next_page‹ wäre also ein besserer Name.

In ›fetch‹, was soll das sleep? Die Funktion ist etwas lang, und sollte wenigstens die innere for-Schleife als Funktion ›parse_article‹ auslagern.
Die vielen if-Abfragen sind alle identisch und sollten durch einen Funktionsaufruf ersetzt werden.

Das Exception-Handling ist völlig ungeeignet. Exceptions immer so konkret wie möglich abfangen. Die meisten Fehler, die auftreten könnten, werden wohl nichts damit zu tun haben, dass die URL falsch ist. Mit der Aussage kann man also den Fehler nie finden. ›exit‹ hat in einem normalen Programm nichts verloren, weil auf diese Weise kann der Aufrufer nichts tun, um den Fehlerfall besser zu verarbeiten. Die beste Fehlerbehandlung ist daher, sie gar nicht zu behandeln.

Da die Klasse eigentlich nur aus einem fetch besteht, handelt es sich nur um eine Funktion.

Die Datei ›crawler.py‹ könnte also so aussehen:

Code: Alles auswählen

from collections import namedtuple
import requests
from bs4 import BeautifulSoup
from urllib.parse import urljoin, urlparse

CrawledArticle = namedtuple("CrawledArticle", "title, content, image, author, data, count_comments")

def get_text(post, selector, default):
    element = post.select_one(selector)
    return default if element is None else element.text

def parse_article(url, post):
    title = get_text(post, ".tp_title", "Kein Title vorhanden!")
    content = get_text(post, "p", "Kein Content vorhanden!")
    author = get_text(post, "li.has-author", "Kein Autor vorhanden!")
    data = get_text(post, "time", "Kein Datum vorhanden!")
    count_comments = get_text(post, "span.count.comment_count", "Keine Anzahl der Kommentare vorhanden!")
    img = post.select_one("img")
    image = urljoin(url, img.attrs["src"]) if img is not None else "Kein Bild vorhanden!"
    return CrawledArticle(title, content, image, author, data, count_comments)

def get_next_url(url, doc):
    url_sub = doc.select_one(".seite_weiter")
    if url_sub is not None:
        return urljoin(url, url_sub.attrs["href"])
    return None

def fetch_articles(url):
    articles = []
    while url:
        r = requests.get(url)
        doc = BeautifulSoup(r.text, "html.parser")
        for post in doc.select("article.news.row"):
            articles.append(parse_article(url, post))
        url = get_next_url(url, doc)
    return articles

und das Hauptprogramm:

Code: Alles auswählen

import csv
import crawler

URL = "https://www.heise.de/tp/energie-und-klima/?seite=1"

def write_to_csv(articles):
    with open('articlesHeisse.csv', 'w', newline='') as csv_file:
        article_writer = csv.writer(csv_file, delimiter=';', quotechar='"', quoting=csv.QUOTE_MINIMAL)

        for article in articles:
            article_writer.writerow(article)

def main():
    write_to_csv(fetch_articles(url))

if __name__ == '__main__':
    main()

EDIT: es war nicht gemeint, für jeden Artikel eine neue Klasse zu erstellen.

[narpfel]

Ich würde auch noch aus `fetch_articles` einen Generator machen und die `writerows`-Methode des Writers benutzen.

Außerdem könnte man `get_next_url` auch in einen Iterator abstrahieren, der über die Seiten iteriert, und damit das Zurückgeben von `None` vermeiden (ungetestet):

Code: Alles auswählen

def iter_pages(url):
    while True:
        doc = BeautifulSoup(requests.get(url).text, "html.parser")
        yield url, doc
        url_sub = doc.select_one(".seite_weiter")
        if url_sub is None:
            return
        url = urljoin(url, url_sub.attrs["href"])
		
def fetch_articles(url):
    for page_url, page in iter_pages(url):
        for post in page.select("article.news.row"):
            yield parse_article(page_url, post)

Ob das jetzt unbedingt besser ist, darüber lässt sich streiten.

[ford]

Danke!

Ich muss noch sehr viel lernen bis ich es annähernd so kann wie ihr