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
Manga downloader
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:
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
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']
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
- birkenfeld
- Python-Forum Veteran
- Beiträge: 1603
- Registriert: Montag 20. März 2006, 15:29
- Wohnort: Die aufstrebende Universitätsstadt bei München
Das sollte man aber auf "del name" einschränken, im Gegensatz zu "del obj[index]".lunar hat geschrieben:Der Gebrauch von "del" dient im Allgemeinen (von Sonderfällen abgesehen) als schlechter Stil.
Oh ja, daran habe ich gar nicht gedacht, weil ich das eigentlich noch nie wirklich benötigt habe …
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().birkenfeld hat geschrieben:Das sollte man aber auf "del name" einschränken, im Gegensatz zu "del obj[index]".
-
- Python-Forum Veteran
- Beiträge: 16025
- Registriert: Freitag 20. Juni 2003, 16:30
- Kontaktdaten:
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.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().
My god, it's full of CARs! | Leonidasvoice vs (former) Modvoice
- veers
- User
- Beiträge: 1219
- Registriert: Mittwoch 28. Februar 2007, 20:01
- Wohnort: Zürich (CH)
- Kontaktdaten:
K, machsch wieder seich =P
Nicht gerade ein vorteilhafter Name.
Mach das entweder Konfigurierbar oder lagere es in Konstanten aus.
self.i += 1
f ist gebräuchlicher.
if not search_result is None: wäre afaik korrekter.
Coding Style: http://www.python.org/dev/peps/pep-0008/
Warum ist das eine Methode und nicht einfach eine Funktion, und warum ist der Code auch in Picture.__init__?
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
Code: Alles auswählen
class Internet
Code: Alles auswählen
def __init__(self):
if not os.path.exists('manga'):
os.mkdir('manga')
def save(self, chapter_path, path='/home/f00'):
Code: Alles auswählen
self.i = self.i + 1
Code: Alles auswählen
fout = open(filename, "wb")
Code: Alles auswählen
if search_result != None:
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)
Code: Alles auswählen
except:
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
"If privacy is outlawed, only outlaws will have privacy." - Phil Zimmermann
- birkenfeld
- Python-Forum Veteran
- Beiträge: 1603
- Registriert: Montag 20. März 2006, 15:29
- Wohnort: Die aufstrebende Universitätsstadt bei München
Ja, pop() existiert und ist dafür da, wenn man den Rückgabewert braucht. Wenn nicht, ist die vorgesehene API "del".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().birkenfeld hat geschrieben:Das sollte man aber auf "del name" einschränken, im Gegensatz zu "del obj[index]".
Yep, hab ich bei meinen SPOJ Experimenten auch herausgefunden.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.
Darf man fragen, warum das hier korrekter sein soll?
zu verwenden (ausgehen von der Lesbarkeit)?
Wäre es nicht sinnvoller:if search_result != None:
>>> if not search_result is None: wäre afaik korrekter.
Code: Alles auswählen
if search_result:
...
Code: Alles auswählen
if search_result is not None:
...
"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.
"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.
@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:
Daher kann ich ja auch ganz einfach ein "if object" verwenden.Match objects always have a boolean value of True
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 ...
- veers
- User
- Beiträge: 1219
- Registriert: Mittwoch 28. Februar 2007, 20:01
- Wohnort: Zürich (CH)
- Kontaktdaten:
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.SchneiderWeisse hat geschrieben:Darf man fragen, warum das hier korrekter sein soll?Wäre es nicht sinnvoller:if search_result != None:
>>> if not search_result is None: wäre afaik korrekter.Code: Alles auswählen
if search_result: ...
zu verwenden (ausgehen von der Lesbarkeit)?Code: Alles auswählen
if search_result is not None: ...
Gruss,
Jonas
[url=http://29a.ch/]My Website - 29a.ch[/url]
"If privacy is outlawed, only outlaws will have privacy." - Phil Zimmermann
"If privacy is outlawed, only outlaws will have privacy." - Phil Zimmermann
Das ist eigentlich wesentlich kürzer:
http://www.python-forum.de/topic-17952. ... ight=manga
http://www.python-forum.de/topic-17952. ... ight=manga