F4Spider - Downloader für FFFFOUND!

[webspider]

Hallo, ich habe vor Ewigkeiten mal in Python 2.x kleine Skripts zur Lösung von nebensächlichen Aufgaben und Online-Herausforderungen geschrieben und mir letztens gedacht erneut einzusteigen. Insbesondere da die Programmierung mit Java an der Uni mir manchmal doch verdammt anstrengend wird.

Also baute ich ein Skript, welches das wahlweise gezielte oder ungezielte Herunterladen von Bildern des "Image Bookmarking Service" FFFFOUND! unterstützt. Alles ist in Python 3.2 geschrieben und bisher ganz gut lauffähig, aber noch alles andere als fix und elegant arbeitend. Von daher fände ich es nett würdet ihr mir mit Verbesserungsvorschlägen kommen.

main.py help.py get.py syn.py

edit: Amüsant, dass mein erstes ernsthaftes Python-Projekt soviel mit meinem Nutzernamen zu tun hat.

[deets]

Ok, ein paar Anmerkungen:

- if <condition> == False: schreibt man stattdessen als if not <condition>:
- die Verarbeitung deiner Kommandos ist sehr gleichfoermig, aber kompliziert und repetitiv ausprogrammiert. Stattdessen solltest du die Behandlung zB in eine Liste von Kommando-Strukturen packen, die immer das Kommando definieren, und dann gucken, ob es

a) ohne argument ist, und dann eines einfordern, so das kommando eines braucht
b) wenn es eines hat, das halt zureckliefern

- "i" als Laufvariable ist sehr schlecht, hiesse es zB "command" waere sofort klar, was es ist, und warum es "exit" werden kann


- fuer Ausgaben "zwischendurch" solltest du logging verwenden, dann kann man die abstellen
- du brauchst keine leeren returns
- return 1 sollte return True sein, bzw eigentlich gar nicht, denn wenn es nur einen konstanent Returnwert "True" bzw. 1 gibt, kann man sich das ja eh sparen
- benutze das with-statement fuer file-like objekte
- syn.py ist ein modul, das aber eine klasse sein sollte. Dann kannst du auch die ganze Methoden wie append, __len__, __contains__ implementieren, und auf sowas wie "has" verzichten. Die Abstraktion ist damit leichter verstaendlich.

[EyDu]

Hallo.

Weil ich mich gerade um meine Arbeit drücken möchte

main.py:
- Du brauchst nicht mit "#imports" kennzeichnen, dass die Importe anfangen, dass sieht man auch so
- Schreibe keinen Code auf Modul-Ebene, packe alles in Funktionen. Sonst kannst du das Modul nicht vernünftig wiederverwenden. Am besten alles in eine main-Funktion packe und so aufrufen:

Code: Alles auswählen

if __name__ == "__main__":
    main()

Dann wird der Code nur ausgeführt, wenn du das Programm aufrufst aber nicht, wenn du es importierst.
- Alles was kein Integer ist sollte nicht an den Namen `i` gebunden werden.
- Die while-Schleife würde man eher als `while(True)` schreiben und bei `exit` mit `break` verlassen.
- Wenn du viele `elif`s hast in denen du immer den selben Namen auf Gleichheit testest, dann solltest du ein Dictionary verwenden und die Fälle in Funktionen aufteilen.
- Du hast überall magische Zahlen stehen, packe diese in Konstanten, sonst weist du in einer Woche schon nicht mehr, was sie bedeuten.
- Wenn du den selben Ausdruck mehrmals schreibst, dann denke über Variablen nach
- Strings werden nicht mit `+` zusammengesetzt, benutze dazu die gelieferten String Formatting Methoden
- Die `print`-Funktion kann mehrere Argumente entgegennehmen, dann spart man sich evtl. das Zusammensetzen von Strings.
- `x == False` solltest du besser als `not x` schreiben.
- Warum gibst du am Anfang immer die Hilfe aus?

help.py:
- Konstanten solltest du durchgehend in Großbuchstaben schreiben (PEP
- Es gibt `__author__` und `__version__`
- Wenn die Datei zu Ende ist, dann weiß auch jeder das nichts mehr kommt. Da ist ein Kommentar der Form "#end of crude doc strings" überflüssig.
- Das "#----------------------------------------" ist reichlich überflüssig, besonders in der ersten Zeile der Datei.
- Warum benutzt du keine Docstrings um den Sinn des Moduls zu beschreiben, sondern Kommentare?

get.py:
- `answer != "yes"` kann man auch freundlicher gestalten: `answer not in ("yes", "y", ...)`
- Die `answer == ` kannst du wieder durch ein Dictionary ersetzen
- Du hast keine Fehlerbehandlung. Wenn jemand keinen Integer eingiebt, dann schmiert das Programm einfach ab
- Ein leeres `return` am Ende einer Funktion ist überflüssig
- Du solltest deine Methoden besser dokumentieren und kommentieren
-

Code: Alles auswählen

for i in range(count):
        h = match[i][22:-1].decode()

solltest du besser so formulieren:

Code: Alles auswählen

for m in match:
        h = m[22:-1].decode()

- Benutze richtige Bezeichner `h` sagt zum Beispiel gar nichts aus.
- `hash` ist eine built-in-Funktion, du solltest daher nichts an diesen Namen binden.
- Ein Parameter mit dem Namen `hash` in der Funktion `hash` ist sehr schlecht gewählt.
- Statt 0 und 1 solltest du `False` und `True` verwenden
- Dateien solltest du mit dem with-Statement öffnen.
- Du vermischt überall Logik mit Eingabe und Ausgabe
- `estimated_archive_size` kann man sicher kürzen, zum Beispiel durch Verwendung von Tupeln und einer Schleife.
- Wenn du einen regulären Ausdruck nur einmal verwendest, dann brauchst du ihn nicht per Hand zu kompilieren
- `page = int((offset + 25) / 25)` denke darüber noch einmal nach
- Wenn `req` die URL nicht öffnen kann wird None zurückgegeben und den Programm bricht ab. Wenn du einen Fehler behandelst, dann musst du dich danach in einem konsistenten Zustand befinden du sorgst jetzt nur dafür, dass es an einer anderen Stelle kracht.
- `convert` kann man viel kürzer schreiben.
- `urlopen` unterstützt sicher auch das with-Statement


syn.py:
- Dateipfade solltest du mit `os.path.join` zusammensetzen
- Benutze keine globalen Variablen wie `db`
- Die Datenbank sollte wohl eher eine Klasse sein und keine Sammlung an Funktionen.


Natürlich gelten die angemerkten Sachen auch in anderen Modulen. Vorallem das with-Statement, das Zusammensetzen von Strings und die Verwendung von Dictionaries.

Es lassen sich noch überall Kleinigkeiten finden, aber bis du die genannten Stellen verbesser hast, hast du sicher genugzu tun.

Sebastian

[webspider]

Oha. Gut, hätt ich mir denken können, dass der Ansatz "Mal schnell was zusammenpacken" auf Dauer nicht gut gehen kann.

Aber vielen Dank für die umfangreiche Liste, ich werde mich dann mal an Objektorientierung, Fehlerbehandlung, Dateiumgang, Abstraktion und die ganzen neuen Features in Python setzen und wenn ich fertig bin Version 0.0.3 hochladen.

Beruhigend, dass das kein perl-Forum ist, was wär da nur los an Unübersichtlichkeit im Code und Meckerei wegen der Formatierung

[webspider]

Ich hab das Skript mal wesentlich vereinfacht. Nach viel Überlegen fiel mir auf, dass ich idealerweise zunächst jede Seite abfrage um an alle Hashes zu kommen, diese in eine Datei abspeichere und dann diese Liste an Hashes durchgehe um die denen zugeordneten Links in einer weiteren Datei zu speichern. So wird das ganze robuster gegenüber Abstürzen.

Die am Ende entstandene Linkliste kann dann an wget verfüttert werden.

So, genug geredet, hier ist das Skript: Pastebin

edit: Hinweise wie man das ganze etwas weniger imperativ aussehen lassen könnte, wären wunderbar. Solange es nicht auf Kosten von Ausführungsgeschwindigkeit geht natürlich.

[Leonidas]

edit: Hinweise wie man das ganze etwas weniger imperativ aussehen lassen könnte, wären wunderbar. Solange es nicht auf Kosten von Ausführungsgeschwindigkeit geht natürlich.

lxml.html zum Parsen und requests zum Runterladen, mit entsprechenden Downloader-Pool. Das wird dadurch sogar schneller UND korrekter