Das deutsche Python-Forum

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

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

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]".

Oh ja, daran habe ich gar nicht gedacht, weil ich das eigentlich noch nie wirklich benötigt habe …

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().

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.

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

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".

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.

Wobei man natürlich in CPython Reference Counting hat, das heißt der Wert muss in jedem Fall ermittelt werden.

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)?

"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.

@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.

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 ...

@lunar: um ehrlich zu sein, hatte ich mir den auch erst im Nachhinein angesehen und hatte einfach Glück...

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.

Gruss,
Jonas

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

Das deutsche Python-Forum

Manga downloader

Manga downloader