Code Aufbau

Wenn du dir nicht sicher bist, in welchem der anderen Foren du die Frage stellen sollst, dann bist du hier im Forum für allgemeine Fragen sicher richtig.
Antworten
mebus08
User
Beiträge: 6
Registriert: Montag 30. Januar 2017, 12:11

Hallo zusammen.
Ich wollte mal nachfragen ob der untenstehende Aufbau so richtig ist. Oder was man ändern soll / muß...
Ändern kann man bestimmt noch einiges aber es führt ja nicht nur 1 weg zum Ziel

Der Code checkt eine ISBN 10 oder 13 Nummer, was bisher auch einwandfrei funktioniert hat (zu mindestens bei mir )

Code: Alles auswählen

def get_clear_isbn(isbn):

    isbn = str(isbn)
    isbn = isbn.lower().replace('-', '').replace(' ', '')

    if len(isbn) == 10:
        if isbn[0:9].isdigit():
            return isbn
        else:
            return 0

    if len(isbn) == 13:
        if isbn.isdigit():
            return isbn
        else:
            return 0
    else:
        return 0


def check_isbn_10(isbn):

    c_isbn = get_clear_isbn(isbn)
    if not c_isbn == 0:
        multi = 1
        checksumme = 0

        for number in c_isbn[::-1]:
            if 'x' in number:
                number = number.replace('x', '10')
            summe = int(number) * multi
            checksumme = checksumme + summe
            multi += 1
        valid = checksumme % 11

        if not valid == 0:
            return False
        else:
            return True
    else:
        return False


def check_isbn_13(isbn):

    c_isbn = get_clear_isbn(isbn)
    if not c_isbn == 0:

        summe1 = 0
        summe2 = 0

        for number in c_isbn[0::2]:
            summe1 = summe1 + int(number)
        summe1 = summe1 * 1

        for number in c_isbn[1::2]:
            summe2 = summe2 + int(number)
        summe2 = summe2 * 3

        checksumme = summe1 + summe2
        valid = checksumme % 10

        if not valid == 0:
            return False
        else:
            return True
    else:
        return False
__deets__
User
Beiträge: 14494
Registriert: Mittwoch 14. Oktober 2015, 14:29

Halbwegs ok. Die Verwendung von 0 ist ungewöhnlich. Nimm stattdessen None, und prüfe mit dem is-operator: if xxx is not None...

Bei der get_clear_isbn Funktion kann der kontrollfluss vereinfacht werden, viele else Zweige sind unnötig. Einfach am Ende Return None, oder nix zurückgeben.

Statt multi anzulegen und zu inkrementieren verwende enumerate:

Code: Alles auswählen

for multi, c in enumerate(isbn, start=1):
    ...

Code: Alles auswählen

if cond:
   return True 
else:
    return False
kann man zu "return cond " bzw im Zweifel "return not cond" bei notwendiger Invertierung vereinfachen.
Benutzeravatar
kbr
User
Beiträge: 1487
Registriert: Mittwoch 15. Oktober 2008, 09:27

@mebus08: Ich habe Dir mal beispielhaft "check_isbn_13" umgeschrieben (ungetestet):

Code: Alles auswählen

def check_isbn_13(isbn):
    clear_isbn = get_clear_isbn(isbn)
    if clear_isbn:
        sum1 = sum(int(n) for n in clear_isbn[::2])
        sum2 = sum(int(n) for n in clear_isbn[1::2]) * 3
        checksum = sum1 + sum2
        return not bool(checksum % 10)
    return False
Sirius3
User
Beiträge: 17711
Registriert: Sonntag 21. Oktober 2012, 17:20

@mebus08: Funktionen sollten im Fehlerfall nicht irgendwelche magischen Werte zurückliefern, sondern eine Exception werfen

Code: Alles auswählen

def get_clear_isbn(isbn):
    isbn = isbn.lower().replace('-', '').replace(' ', '')
    if len(isbn) == 10 and isbn[:9].isdigit():
        return isbn
    if len(isbn) == 13 and isbn.isdigit():
        return isbn
    raise ValueError(isbn)
In diesem Fall ist aber gar nicht klar, was get_clear_isbn überhaupt machen soll. Man kann check_isbn_10 mit einer ungültigen ISBN13 aufrufen und trotzdem True als Ergebnis bekommen. Umgekehrt natürlich auch.

Bei check_isbn_10 ist die in-Prüfung bei einem String der genau ein Zeichen enthält nicht gut, ebenso das replace bei einem String, der immer identisch ist mit dem zu ersetzenden Text. Das Ergebnis einer Multiplikation "summe" zu nennen, ist auch verwirrend.

Code: Alles auswählen

def check_isbn_10(isbn):
    isbn = isbn.lower().replace('-', '').replace(' ', '')
    if len(isbn) == 10 and isbn[:9].isdigit():
        checksumme = sum( (10 if number == 'x' else int(number)) * magic
            for magic, number in enumerate(c_isbn[::-1], 1))
        return checksumme % 11 == 0
    return False
mebus08
User
Beiträge: 6
Registriert: Montag 30. Januar 2017, 12:11

Ich habe es jetzt mal umgeschrieben und mit kommentaren versehen(bitte korrigieren wenn es falsch sein sollte)

Code: Alles auswählen

def get_clear_isbn(isbn):
    # wandelt in str um falls int eigegebn wird
    isbn = str(isbn)
    # entfehrent '-' und ' ', wandelt groß in kleinschreibung
    return isbn.lower().replace('-', '').replace(' ', '')


def check_isbn_10(isbn):
    # gibt gesäuberte isbn wieder
    c_isbn = get_clear_isbn(isbn)
    # prüft auf länge 10 und ob die ersten 9 zeichen zahlen sind
    if len(c_isbn) == 10 and c_isbn[0:9].isdigit():
        # prüft ob das lezte zeichen eine zahl ist
        if c_isbn[-1:].isdigit():
            # summiert alle Zahlen zusammen
            result = sum(int(number) * result for result,
                         number in enumerate(c_isbn[::-1], 1))
            # gibt True zurück wenn Modulo = 0 ist
            return result % 11 == 0
        # prüft ob das letzt zeichen ein 'x' ist    
        elif c_isbn[-1:] == 'x':
            # summiert alle zahlen zusammen
            result = sum(int(number) * result for result,
                         number in enumerate(c_isbn[0:9][::-1], 2)) + 10
            #gibt True zurück wennn Modulo = 0 ist
            return result % 11 == 0
    # gibt False zurück wenn keine der Anweisungen stimmen        
    return False


def check_isbn_13(isbn):
    # gibt gesäuberte isbn wieder
    c_isbn = get_clear_isbn(isbn)
    #prüft auf 13 zeichen und ob alle zahlen sind
    if len(c_isbn) == 13 and c_isbn.isdigit():
        # summiert jede 2 stelle ab dem 1 zeichen
        result_1 = sum(int(number) for number in c_isbn[0::2])
        # summiert jede 2 stelle ab den 2 zeichen * 3
        result_2 = sum(int(number) for number in c_isbn[1::2]) * 3
        # ergebnis aus beiden summen
        valid = result_1 + result_2
        # gibt True zurück wenn Modulo = 0 ist
        return valid % 10 == 0

    else:
        #Gibt False zurück wenn keine der Anweisung stimmen
        return False
__deets__
User
Beiträge: 14494
Registriert: Mittwoch 14. Oktober 2015, 14:29

Die Kommentare sind in der Form mE überflüssig, da sie eh nur erklären, was danach im Code eh passiert.

Die Fallunterscheidung für das x ist zu groß. Besser wäre einen Wert zu berechnen, der dann zur Summe addiert wird - dann sind summierung und return nicht gedoppelt.
Benutzeravatar
kbr
User
Beiträge: 1487
Registriert: Mittwoch 15. Oktober 2008, 09:27

@mebus08: Die geantworteten Funktionen sind "pythonisch", ohne Redundanz und mit klaren Bezeichnern. Warum hast Du sie umgeschrieben?
Benutzeravatar
sls
User
Beiträge: 480
Registriert: Mittwoch 13. Mai 2015, 23:52
Wohnort: Country country = new Zealand();

@mebus08: Du hast zwei Funktionen, die im Grunde das selbe tun. Ich würde darüber nachdenken, ob es nicht sinnvoll ist diese zusammen zu fassen.

get_clear_isbn() klingt so, als würde es den der Funktion zu übergebenden Parameter zurücksetzen, löschen etc. - ich würde die Funktion so benennen, dass sofort klar wird was sie tut. "format_isbn", "convert_isbn_string" - vielleicht in diese Richtung.

Es gibt immernoch keinen Schutzmechanismus, um 'ValueError' abzufangen, ist das gewollt? Generell lässt sich so ja nur prüfen, ob es sich um eine ISBN Nr. 10, 13 oder nichts handelt, bei letzterem ist aber nicht wirklich klar, warum die Nummer keiner der ersteren beiden entspricht.

Bei den Kommentaren stört mich persönlich, dass sie auf deutsch sind, da deine Funktionsbezeichnungen und Variablen in Englisch geschrieben sind, und du nahezu jede Zeile kommentierst. Das ist IMO nicht notwendig, da man i.d.R. wissen sollte, dass ein Wert '0' sein wird, wenn 'return result % 11 == 0' ist. Ich würde das evtl. so machen:

Code: Alles auswählen

check_isbn_10():
    """This function does bla bla bla.""""
    code logic here

Möchtest du nun aus einem anderen Programm heraus schnell nachlesen, was 'check_isbn_10()' macht, kannst du das über die hinterlegten docstrings der Funktion bequem und schnell in Erfahrung bringen. Dazu mehr unter https://www.python.org/dev/peps/pep-0257/.

Natürlich kannst du weiterhin mit einfachen Kommentaren Komplexe Sachverhalte kommentieren, sollte hier aber nicht die Regel sein.
When we say computer, we mean the electronic computer.
mebus08
User
Beiträge: 6
Registriert: Montag 30. Januar 2017, 12:11

@kbr
ich wollte halt nicht einfach nur kopieren und einfügen. da ist der Lerneffekt doch gleich null oder nicht???

@__deets__
die kommentare sind eigentlich erstmal nur für mich gedacht.

@sls
mit der umbennenung der funktion get_clear_isbn nach format_isbn hast du recht.

ist es denn nicht besser 2 Funktionen mit eindeutigem namen zu haben als eine???
den ValueError werde ich noch einbauen.
die Kommentare sind wie oben schon geschrieben erstmal nur für mich.
Benutzeravatar
sls
User
Beiträge: 480
Registriert: Mittwoch 13. Mai 2015, 23:52
Wohnort: Country country = new Zealand();

mebus08 hat geschrieben: ist es denn nicht besser 2 Funktionen mit eindeutigem namen zu haben als eine???
Hi,

nein, weil die Prüfung der ISBN sich nur in der Länge der Ziffern unterscheidet, ansonsten nicht wirklich. Um eine ISBN '10' oder '13' über deine frühere get_clear_isbn() zu formatieren, hast du ja auch nicht zwei unterschiedliche Funktionen erstellt, sondern nur die eine. So solltest du das auch für das Prüfen der ISBN betrachten und in einem späteren Schritt check_isbn_10 und check_isbn_13 zusammenführen.

Hintergrund ist, dass du so gewährleistet, dass dein Code nicht unnötig aufgebläht wird, leicht zu verstehen, skalierbar und performanter ist. Stell dir z.B. vor, du möchtest 100 oder 100.000 verschiedene ISBN prüfen. Du müsstest mit deinem bisherigen Code also ebenso viele ISBN-Check-Funktionen schreiben.

Mfg,

sls
When we say computer, we mean the electronic computer.
Sirius3
User
Beiträge: 17711
Registriert: Sonntag 21. Oktober 2012, 17:20

@sls: es gibt aber nicht 100 oder 100000 verschiedene ISBN-Formate, und falls es die gäbe, müßte man auch so viele Funktionen schreiben. Unter format_isbn würde ich das Gegenteil verstehen, also aus einer Folge von Ziffern eine mit "-" abgetrennte Gruppe von ISBN-Ziffern. Der Name clear_isbn sagt dagegen genau das aus, was die Funktion macht (das get_ ist ein bißchen irreführend, weil ja nichts von irgendwo geholt wird, sondern nur die Eingabe in eine Ausgabe verwandelt wird).

@mebus08: Python ist an sich sehr gut lesbar, so dass Deine Kommentare meist wirklich nur die wörtliche Übersetzung der Zeile darunter ist. Das letzte Zeichen ist übrigens `c_isbn[-1]`. Was soll übrigens das c_-Präfix? Das c steht wahrscheinlich für cleared, aber das muß man raten oder aber den Code durchsuchen, wo es denn eingeführt wird. Namen sollten sofort, ohne dass man groß den Kontext kennen muß, sagen, was sie enthalten. Hier kann ohne Verlust das c_ einfach weggelassen werden. Bei Deinen Check-Funktionen solltest Du die Bedingungen umdrehen und im Fehlerfall möglichst früh mit `return False` aussteigen. Weniger Einrückung macht den Code lesbarer:

Code: Alles auswählen

def check_isbn_10(isbn):
    """ checks if a number or string is a valid ISBN10 """
    isbn = clear_isbn(isbn)
    if len(isbn) != 10 or not isbn[:9].isdigit() or isbn[10] not in '0123456789x':
        return False
    last_digit = 10 if isbn[10] == 'x' else int(isbn[10])
    checksum = sum(int(number) * place
        for place, number in enumerate(isbn[:9::-1], start=2))
    return (checksum + last_digit) % 11 == 0
Benutzeravatar
snafu
User
Beiträge: 6731
Registriert: Donnerstag 21. Februar 2008, 17:31
Wohnort: Gelsenkirchen

Wobei der Doctstring ("check if a number or string (...)") nicht wirklich zur Funktion passt. Wenn man tatsächlich eine Zahl übergibt, dann wirft die Funktion einen Fehler.
Sirius3
User
Beiträge: 17711
Registriert: Sonntag 21. Oktober 2012, 17:20

@snafu: Die clear-Funktion wandelt Zahlen in Strings um, wobei natürlich keine führenden Nullen berücksichtigt werden, was die Anwendbarkeit doch stark einschränkt.
Benutzeravatar
snafu
User
Beiträge: 6731
Registriert: Donnerstag 21. Februar 2008, 17:31
Wohnort: Gelsenkirchen

Die erste Zeile hab ich wohl überlesen. Alles gut... :oops:
Sirius3 hat geschrieben:(...) wobei natürlich keine führenden Nullen berücksichtigt werden, was die Anwendbarkeit doch stark einschränkt.
Bei führenden Nullen kann man einfach keine Zahl übergeben aus technischen Gründen. Das ist in anderen Bereichen ja genau so.
Benutzeravatar
sls
User
Beiträge: 480
Registriert: Mittwoch 13. Mai 2015, 23:52
Wohnort: Country country = new Zealand();

Sirius3 hat geschrieben:@sls: es gibt aber nicht 100 oder 100000 verschiedene ISBN-Formate, und falls es die gäbe, müßte man auch so viele Funktionen schreiben. Unter format_isbn würde ich das Gegenteil verstehen, also aus einer Folge von Ziffern eine mit "-" abgetrennte Gruppe von ISBN-Ziffern. Der Name clear_isbn sagt dagegen genau das aus, was die Funktion macht (das get_ ist ein bißchen irreführend, weil ja nichts von irgendwo geholt wird, sondern nur die Eingabe in eine Ausgabe verwandelt wird).
Hi,

ok, d.h. wenn ich z.B. 50 ISBN habe, lasse ich alle zunächst durch die clear_isbn() laufen und muss diese anschließend erst durch check_isbn_10 und dann durch check_isbn_13 laufen lassen, wenn ich das richtig sehe.

Mir geht's halt darum, dass ich mit einer Funktion 'check_isbn()' diese Unterscheidung im ausführenden Programmteil nicht treffen muss, da die Daten nicht doppelt geprüft werden müssen. Vorausgesetzt ich habe jetzt nicht etwas Grundlegendes übersehen.

Das aber nur zum Verständnis, ich hoffe den TE dadurch nicht zusätzlich zu verwirren.

Mfg,

sls
When we say computer, we mean the electronic computer.
Benutzeravatar
kbr
User
Beiträge: 1487
Registriert: Mittwoch 15. Oktober 2008, 09:27

@sls: Die Prüfung der ISBN kann etwa so ausschauen:

Code: Alles auswählen

def is_valid_isbn(isbn):
    isbn = get_clear_isbn(isbn):
    isbn_length = len(isbn)
    if isbn_length == 10:
        return check_isbn_10(isbn)
    if isbn_length == 13:
        return check_isbn_13(isbn)
    return False
so dass auf den Aufruf von 'get_clear_isbn' innerhalb der check_isbn-Funktionen verzichtet werden kann. Es braucht also nichts doppelt geprüft werden. Je nach Programmentwurf kann es auch sinnvoll sein 'get_clear_isbn' außerhalb von 'is_valid_isbn' aufzurufen, falls diese irgendwo weiter verwendet werden soll.
mebus08
User
Beiträge: 6
Registriert: Montag 30. Januar 2017, 12:11

Sorry das ich mich erst jetzt wieder melde.... viel arbeit.... egal.

So ich habe die Funktionen jetzt in eine zusammengepackt.

Code: Alles auswählen

def check_isbn(isbn):
    '''
    Validate ISBN-10 and ISBN-13 Numbers
    Return True or False
    '''

    isbn = str(isbn)
    isbn = isbn.lower().replace('-', '').replace(' ', '')

    if len(isbn) == 10:
        if not isbn[0:9].isdigit() or isbn[9] not in '0123456789x':
            return False
        else:
            last_char = 10 if isbn[9] == 'x' else int(isbn[9])
            result = sum(int(number) * result for result,
                         number in enumerate(isbn[0:9][::-1], 2))
            return (last_char + result) % 11 == 0
    if len(isbn) == 13:
        if not isbn.isdigit():
            return False
        else:
            result_1 = sum(int(number) for number in isbn[0::2])
            result_2 = sum(int(number) for number in isbn[1::2]) * 3
        return (result_1 + result_2) % 10 == 0
    return False
    # raise ValueError('No correct ISBN')
aber eine frage habe ich nochmals wegen der Fehlerbehandlung.
Ist es denn nicht einfacher auf true oder false zu reagieren wenn man die funktion aufruft?

Code: Alles auswählen

if not check_isbn('123456789k'):
    print('Die eingegebne ISBN ist nicht richtig')
else:
   mache weiter
__deets__
User
Beiträge: 14494
Registriert: Mittwoch 14. Oktober 2015, 14:29

Mir fehlt bei deiner Frage die Alternative. Einfacher als was? Meinst du exceptions? Wenn ja: kommt drauf an. Wenn du eine sehr lokale Fehlerbehandlung durchführen kannst, kann man auch nur den rückgaberwert prüfen. Wenn du aber ggf. irgendwo tief in einer Verarbeitung steckst, die dann abgebrochen werden muss, dann ist eine Exception die bessere Wahl.

Beides kann ja aber leicht ineinander überführt werden.
Sirius3
User
Beiträge: 17711
Registriert: Sonntag 21. Oktober 2012, 17:20

@mebus08: es kommt darauf an, was Du aussagen willst. Der Name der Funktion check_isbn läßt vermuten, dass ein Wahrheitswert zurückgeliefert wird, es kann aber auch sein, dass offensichtlich falsche Eingaben auf einen Programmierfehler hinweisen und deshalb eine Exception geworfen wird. Das ist z.B. dann der Fall, wenn der Parameter vom falschen Typ ist, kann aber auch sein, wenn der String nicht die richtige Länge oder nicht die richtigen Zeichen enthält. Soll also geprüft werden, ob eine ISBN-Nummer die richtige Checksumme hat? Oder ob ein beliebiger String eine korrekte ISBN-Nummer ist?
Benutzeravatar
snafu
User
Beiträge: 6731
Registriert: Donnerstag 21. Februar 2008, 17:31
Wohnort: Gelsenkirchen

Es gibt halt Mischformen. So könnte es bei der Übergabe einer Zeichenkette immer einen Wahrheitswert als Rückgabe geben, egal wie unsinnig die Eingabe möglicherweise war. Wenn aber ein komplett falscher Typ übergeben wird, dann könnte man immer noch eine Exception werfen (lassen). Solche Exceptions sollten dann IMHO auch zum Programmabbruch führen oder zumindest zu einem Fehler auf der oberen Ebene, sofern noch eine reelle Chance besteht, das Problem zu lösen. Meistens deutet das auf einen Programmierfehler hin. Wenn der mehr oder weniger "verschluckt" wird, dann besteht eine große Gefahr von Folgefehlern, deren Ursache nur noch schwer nachzuvollziehen ist.
Antworten