Manga downloader

Code-Stücke können hier veröffentlicht werden.
Antworten
suizo
User
Beiträge: 3
Registriert: Donnerstag 2. Juli 2009, 10:18

Hallo Zusammen

Ich bin rel. neu hier :)

und dies ist mein erstes skript in py.

es macht nichts anderes als auf onemanga.com zu gehen und dort von einem beliebigen manga die Bilder herunterzuladen (numeriert von 1-n).

wie schon erwähnt ist dies mein erstes skript daher denke ich, dass ich noch einiges verbessern kann.

hier der code:
http://paste.pocoo.org/show/135967/

um es auszuführen müsst ihr nur der classe 'internet' ein manganamen übergeben. zb: 'Kampfer'

Gruss
suizo
lunar

Der Quelltext enthält viele überflüssige Sachen. Dazu zählen die "del"-Anweisungen. Da Python den Speicher automatisch verwaltet, musst Du ihn nicht manuell freigeben. Der Gebrauch von "del" dient im Allgemeinen (von Sonderfällen abgesehen) als schlechter Stil. Desweiteren sind die "Deklarationen" in Zeile 7, 8 und 72 unnötig. In Python müssen Variablen nicht deklariert werden, auch wenn es Brauch ist, die Exemplarattribute geschlossen in "__init__()" zu definieren. Die Zeilen in Deinem Code erzeugen übrigens auch keine Exemplarattribute, sondern Klassenattribute, wie Du an folgendem Beispiel sehen kannst:

Code: Alles auswählen

class Spam(object):
    eggs = []

spam_one = Spam()
spam_one.eggs.append('hello world')
spam_two = Spam()
print spam_two.eggs
# Ausgabe:  ['hello world']
Die Klasse "Internet" ist in ihrer gegenwärtigen Form ebenfalls unnütz. Ihre Methoden teilen keinerlei sinnvolle Exemplarattribute. Das ist keine Klasse, die ein in sich abgeschlossenes Objekt repräsentieren kann, sondern eine Sammlung von Funktionen im Klassengewand.

Desweiteren gibt es einige kleinere Kritikpunkte. Dein Code ist plattformabhängig: "c:\tmp" gibt es nur unter Windows. Und selbst unter Windows liegt das temporäre Verzeichnis nicht zwangsläufig unter diesem Pfad. Nutze das "tempfile"-Modul zum Zugriff auf das temporäre Verzeichnis. Im Allgemeinen gibt es den Backslash als Verzeichnistrenner auch nur unter Windows. Du solltest daher "os.path.join()" verwenden, um Pfade zusammenzusetzen. Das sorgt automatisch dafür, dass der Pfad entsprechend dem System zusammengesetzt wird. Angesichts dieser Plattformabhängigkeit verstehe ich auch nicht, wieso dann plötzlich "/home/f00" auftaucht.

Außerdem schließt Du Dateien nicht korrekt. Lese Ausnahmebehandlung und die "with"-Anweisung in der Dokumentation und im Tutorial nach.

Für reguläre Ausdrücke solltest Du sogenannte "Raw-Strings" verwenden.

"connect_to_internet()" finde ich merkwürdig. Derartige Tests sind unnötig, verwende einfach gleich den ProxyHandler. Falls kein Proxy definiert ist, tut der meines Wissens einfach nichts.

Den überflüssigen Import "progressbar" kannst Du auch noch entfernen. Zumindest solltest Du aber verraten, woher "progressbar" kommt, denn in der Standardbibliothek gibt es dieses Modul nicht :)
Benutzeravatar
birkenfeld
Python-Forum Veteran
Beiträge: 1603
Registriert: Montag 20. März 2006, 15:29
Wohnort: Die aufstrebende Universitätsstadt bei München

lunar hat geschrieben:Der Gebrauch von "del" dient im Allgemeinen (von Sonderfällen abgesehen) als schlechter Stil.
Das sollte man aber auf "del name" einschränken, im Gegensatz zu "del obj[index]".
Dann lieber noch Vim 7 als Windows 7.

http://pythonic.pocoo.org/
lunar

Oh ja, daran habe ich gar nicht gedacht, weil ich das eigentlich noch nie wirklich benötigt habe …
Benutzeravatar
snafu
User
Beiträge: 6738
Registriert: Donnerstag 21. Februar 2008, 17:31
Wohnort: Gelsenkirchen

birkenfeld hat geschrieben:Das sollte man aber auf "del name" einschränken, im Gegensatz zu "del obj[index]".
Würde man das nicht eher mit ``obj.pop(index)`` lösen? Ich wollte auch zuerst als Beispiel das Löschen von Schlüsseln aus einem Dictionary nennen, aber auch dort gibt es bekanntlich pop().
Leonidas
Python-Forum Veteran
Beiträge: 16025
Registriert: Freitag 20. Juni 2003, 16:30
Kontaktdaten:

snafu hat geschrieben:Würde man das nicht eher mit ``obj.pop(index)`` lösen? Ich wollte auch zuerst als Beispiel das Löschen von Schlüsseln aus einem Dictionary nennen, aber auch dort gibt es bekanntlich pop().
Ich würde auch eher ``pop`` nutzen, jedoch ist ``del`` effizienter, weil der Wert des gelöschten Schlüssels nicht ermittelt und zurückgegeben werden muss.
My god, it's full of CARs! | Leonidasvoice vs (former) Modvoice
Benutzeravatar
veers
User
Beiträge: 1219
Registriert: Mittwoch 28. Februar 2007, 20:01
Wohnort: Zürich (CH)
Kontaktdaten:

K, machsch wieder seich =P

Code: Alles auswählen

class Internet
Nicht gerade ein vorteilhafter Name.

Code: Alles auswählen

    def __init__(self):
	if not os.path.exists('manga'):
	    os.mkdir('manga')

    def save(self, chapter_path, path='/home/f00'):
Mach das entweder Konfigurierbar oder lagere es in Konstanten aus.

Code: Alles auswählen

	self.i = self.i + 1
self.i += 1

Code: Alles auswählen

            fout = open(filename, "wb")
f ist gebräuchlicher.

Code: Alles auswählen

if search_result != None:
if not search_result is None: wäre afaik korrekter.

Coding Style: http://www.python.org/dev/peps/pep-0008/

Code: Alles auswählen

    def manga_folder_exist(self):
        if not os.path.exists('manga'):
            os.mkdir("manga")
        if not os.path.exists('manga/'+self.manga_name):
            os.mkdir("manga/"+self.manga_name)   
Warum ist das eine Methode und nicht einfach eine Funktion, und warum ist der Code auch in Picture.__init__?

Code: Alles auswählen

        except:
Except ohne Bedingung ist böse, das fängt alles von SystemExit zum "out of memory" Fehlern. Verwende zumindest except Exception:

Ausserdem wenn du except: ... raise hast kannst du auch gleich finally verwenden.

Das dürfte genügend genörgel sein für heute. ;)

Zum 'Design',
Ich hätte bei so einem Script gar keine Klassen definiert. 2-3 Funktionen reichen in dem Fall bei weitem.

So ich denke das ist genug genörgelt für heute.

Gruss,
Jonas
[url=http://29a.ch/]My Website - 29a.ch[/url]
"If privacy is outlawed, only outlaws will have privacy." - Phil Zimmermann
Benutzeravatar
birkenfeld
Python-Forum Veteran
Beiträge: 1603
Registriert: Montag 20. März 2006, 15:29
Wohnort: Die aufstrebende Universitätsstadt bei München

snafu hat geschrieben:
birkenfeld hat geschrieben:Das sollte man aber auf "del name" einschränken, im Gegensatz zu "del obj[index]".
Würde man das nicht eher mit ``obj.pop(index)`` lösen? Ich wollte auch zuerst als Beispiel das Löschen von Schlüsseln aus einem Dictionary nennen, aber auch dort gibt es bekanntlich pop().
Ja, pop() existiert und ist dafür da, wenn man den Rückgabewert braucht. Wenn nicht, ist die vorgesehene API "del".
Dann lieber noch Vim 7 als Windows 7.

http://pythonic.pocoo.org/
Benutzeravatar
hendrikS
User
Beiträge: 420
Registriert: Mittwoch 24. Dezember 2008, 22:44
Wohnort: Leipzig

Leonidas hat geschrieben:Ich würde auch eher ``pop`` nutzen, jedoch ist ``del`` effizienter, weil der Wert des gelöschten Schlüssels nicht ermittelt und zurückgegeben werden muss.
Yep, hab ich bei meinen SPOJ Experimenten auch herausgefunden.
Benutzeravatar
Trundle
User
Beiträge: 591
Registriert: Dienstag 3. Juli 2007, 16:45

Wobei man natürlich in CPython Reference Counting hat, das heißt der Wert muss in jedem Fall ermittelt werden.
"Der Dumme erwartet viel. Der Denkende sagt wenig." ("Herr Keuner" -- Bertolt Brecht)
nemomuk
User
Beiträge: 862
Registriert: Dienstag 6. November 2007, 21:49

Darf man fragen, warum das hier korrekter sein soll?
if search_result != None:
>>> if not search_result is None: wäre afaik korrekter.
Wäre es nicht sinnvoller:

Code: Alles auswählen

if search_result:
    ...

Code: Alles auswählen

if search_result is not None:
    ...
zu verwenden (ausgehen von der Lesbarkeit)?
lunar

"not something is None" und "something is not None" sind äquivalent. Ich persönlich bevorzuge die zweite Variante, aber meiner Meinung nach ist das Geschmacksfrage.

"something" dagegen ist etwas anderes als "something is not None" und kann daher nicht als Ersatz für einen Test auf "None" empfohlen werden.
Zuletzt geändert von lunar am Samstag 29. August 2009, 11:16, insgesamt 1-mal geändert.
nemomuk
User
Beiträge: 862
Registriert: Dienstag 6. November 2007, 21:49

@lunar: ja, das ist schon klar, dass das etwas anderes ist, aber in diesem Fall doch irrelevant. re.search liefert entweder None oder ein Match-Objekt:
Match objects always have a boolean value of True
Daher kann ich ja auch ganz einfach ein "if object" verwenden.
lunar

Ich wusste nicht, welchen Typs "search_result" ist, so genau habe ich mir den Quelltext des OP nicht angesehen. Das Resultat einer Suche kann schließlich alles mögliche sein, auch eine Liste mit Ergebnissen ;) Bei einem Match-Objekt aber würde ich auch auf den None-Test verzichten und die einfache Form wählen. Allerdings ist das imho mehr eine Stilfrage ...
nemomuk
User
Beiträge: 862
Registriert: Dienstag 6. November 2007, 21:49

@lunar: um ehrlich zu sein, hatte ich mir den auch erst im Nachhinein angesehen und hatte einfach Glück...;)
Benutzeravatar
veers
User
Beiträge: 1219
Registriert: Mittwoch 28. Februar 2007, 20:01
Wohnort: Zürich (CH)
Kontaktdaten:

SchneiderWeisse hat geschrieben:Darf man fragen, warum das hier korrekter sein soll?
if search_result != None:
>>> if not search_result is None: wäre afaik korrekter.
Wäre es nicht sinnvoller:

Code: Alles auswählen

if search_result:
    ...

Code: Alles auswählen

if search_result is not None:
    ...
zu verwenden (ausgehen von der Lesbarkeit)?
Weil er prüfen will ob search_result None IST und nicht gleich wie None ist. Aber du hast recht, in dem Falle ist ein einfaches if die beste Lösung. :wink:

Gruss,
Jonas
[url=http://29a.ch/]My Website - 29a.ch[/url]
"If privacy is outlawed, only outlaws will have privacy." - Phil Zimmermann
Benutzeravatar
cryzed
User
Beiträge: 82
Registriert: Samstag 28. März 2009, 15:53

Das ist eigentlich wesentlich kürzer:
http://www.python-forum.de/topic-17952. ... ight=manga
Antworten