Das deutsche Python-Forum

Ein Script zur Berechnung des ed2k Hashwertes:

http://code.pytech.org/?page=show_paste&id=14

Wenn einer Verbesserungsvorschläge hat, immer her damit.

Und könnte es jemand unter Linux testen, plz?

MfG, Craven

Craven hat geschrieben:Und könnte es jemand unter Linux testen, plz?

Wo erwartest du denn Probleme? Ich konnte beim überfliegen zumindest keine Probleme diesbezüglich finden.

Dafür aber andere: der Code ist nicht PEP8 konform, enthält keinerlei brauchbare Kommentare, dafür aber Funktionen mit nichtssagenden Namen und seltsamen Parametern (zum Beispiel würde man ``method`` eher mit None belegen und anders, einleuchtender benennen). Einen String ``string`` zu benennen ist ebenso ungeschickt, weil man dann das Modul ``string`` nicht importieren kann. Von der Einrückung mal gar nicht zu sprechen.

Hallo,

ich bin zwar nicht der beste Code Reviewer für Python, aber für meinen Geschmack hast du einen ziemlichen Hang zur Code Duplizierung.

Sowohl md4_hash_file und md4_hash, als auch hash_file und hash_file2 machen vom Kern her das Gleiche. Die Namen könnten klarer sein.

Code: Alles auswählen

chunksize = 9728000
if os.path.getsize(filename) <= chunksize:
    return self.md4_hash_file(filename, True)
ed2k_hash = ""
fp = file(filename, 'rb')
content = True
while content != "":
    content = fp.read(chunksize)
    if content != "":
        ed2k_hash += self.md4_hash(content)

Code: Alles auswählen

if method == True:
    return hasher.hexdigest()
else:
    return hasher.digest()

Dieser Code ist gleich. Den hättest du in eine eigene Funktion auslagern können. Sowas wird dann Refactoring genannt.

File open und close hättest du an einer zentralen Stelle ausführen können, z. Bsp vor der if Anweisung. Dafür müsstest du einen else Block einbauen.
Aus md4_hash_file könntest du open und close entfernen. Wenn du die Arbeit ausgeführt hast, sollte dir was auffallen.

Die if Anweisung im While Scope scheint mir unnötig.

Dann drängt sich mir noch die Frage auf, was ist den die Aufgabe der Klasse ed2k?

Na ja, hoffentlich war nun nicht zu streng.

Gruß
Markus

Die Verwendung von Docstrings wäre vorteilhaft.

So, jetzt aber im richtigen Thema...

Craven hat geschrieben:Wenn einer Verbesserungsvorschläge hat, immer her damit.

Aber gerne doch!

Das hier ist mit Sicherheit nicht richtig:

Code: Alles auswählen

class ed2k():

Zudem braucht es hier wirklich keine Klasse (auch wenn man sich manchmal merkwürdig dazu gedrängt fühlt). Es ist vollkommen ausreichend, hier nur Funktionen zu verwenden. Dadurch fallen eine Einrücktiefe und diverse `self`s weg, was die Lesbarkeit verbessert. Als Modul `ed2k` hat es bereits einen Namespace, wenn man es importiert, daher ist auch deshalb die Klasse nicht sinnvoll (in der dunklen PHP-Welt sieht das anders aus, aber hier scheint die Sonne).

Code: Alles auswählen

if method == True:

Hier wird ein Wahrheitswert erwartet, der vom Vergleichsoperator geliefert wird. Allerdings geht diese Bedingung davon aus, dass `method` wahr oder falsch ist bzw. behandelt es einfach wie einen Wahrheitswert. Daher reicht dieser Code völlig:

Code: Alles auswählen

if method:

Hier kann man Klammern zur besseren Lesbarkeit verwenden:

Code: Alles auswählen

if (filesize % chunksize) == 0:

Würde hier zudem der Ungleich-Operator `!=` verwendet, könnte man den Teil `== 0` ähnlich dem vorigen Beispiel weglassen, da `0` zu `False` ausgewertet würde.

Mir persönlich hat es das mit Python 2.5 eingeführte `with`-Statement sehr angetan. Aus

Code: Alles auswählen

fp = file(filename, 'rb')
hasher = hashlib.new("md4")
hasher.update(fp.read())
fp.close()

würde dann

Code: Alles auswählen

# wird in 2.6 nicht mehr erforderlich
from __future__ import with_statement 

with file(filename, 'rb') as fp:
    hasher = hashlib.new("md4")
    hasher.update(fp.read())

So ist sichergestellt, dass die Datei in jedem Fall wieder beim Verlassen des Blocks geschlossen wird.

Code: Alles auswählen

# ...
    ed2k_hash += self.md4_hash(content)
# ...

Will man einen String zusammenfügen, sollte man die Teilstücke in eine Liste speichern und diese dann am Ende mit `''.join(string_parts)` auf einmal zusammenfügen. Da Strings keine Array-ähnlichen Typen, sondern atomar und immutable sind, müssen sie bei jedem `+=` neu angelegt werden, was sich in geringerer Performance widerspiegelt. Dazu gibt es hier im Forum auch mehrere Themen, die das näher beleuchten.

Ansonsten fällt meinem spitzfindigen Auge auf, dass deine Einrücktiefe und die Methodensignatur bei Keywords nicht den Empfehlungen von PEP 8 folgen. Auch solltest du einheitliche String-Delimiter (`'` oder `"`) verwenden und nicht abwechseln. Docstrings für die Funktionen wären natürlich auch schön.

Y0Gi hat geschrieben:Das hier ist mit Sicherheit nicht richtig:
Code: Alles auswählen
class ed2k():

Naja, es ist das Aequivalent zu class ed2k:, die Klammern sind halt ueberfluessig. Falsch ist es aber nicht (bis auf die Tatsache, dass es in Python < 2.5 einen SyntaxError verursacht).

lumax hat geschrieben:
Y0Gi hat geschrieben:Das hier ist mit Sicherheit nicht richtig:
Code: Alles auswählen
class ed2k():
Naja, es ist das Aequivalent zu class ed2k:, die Klammern sind halt ueberfluessig. Falsch ist es aber nicht (bis auf die Tatsache, dass es in Python < 2.5 einen SyntaxError verursacht).

Ich find diese Form sogar ganz nett, weil ich mir schon lange überlegt habe warum es ``def foo():`` aber ``class Foo:`` ist. Klar, sind nicht direkt so vergleichbar, aber ich finds gut, dass die jetzt mehr oder minder konsistent sind.

Ach stimmt, diese die-Klammern-sind-da-aber-die-Klasse-erbt-nicht-explizit-Geschichte? Gut, so gesehen passt das in der Tat. Ich habe das nur auf Anhieb für einen fälschlichen Funktionsaufruf gehalten, aber selbst das ist ja mit dieser Sichtweise nachvollziehbar.

Das deutsche Python-Forum

ed2k hash

ed2k hash

Re: ed2k hash

Re: ed2k hash

Re: ed2k hash

Re: ed2k hash