Verbesserung Tipps für mein erstes Python-Projekt (Programmierstil)

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
malin
User
Beiträge: 11
Registriert: Donnerstag 9. Februar 2017, 14:24

Hallo Community :D

Ich arbeite an meinem ersten kleinen Projekt, einen Sudokulöser der möglichst schnell arbeiten soll.
Ich wollte euch bitten, mir Ratschläge zu geben, wie ich mein Programmierstil verbessern kann.
(So beim schnellen drüberfliegen)
Es dürfen natürlich auch Ratschläge gemacht werden, wie der Algorithmus Optimiert werden kann,
oder mit welche Datenstrukturen sich besser für ein Problem eignen, aber ich denke, dass
es etwas zu viel zu unübersichtlicher Code ist, als das ich euch das zumuten könnte :mrgreen:

Code: Alles auswählen

def sudokuloeser3(S):
    
    def kandidatenListe_init(S, BZS):
        """
        erstellt eine 9x9 kandidatenListe zu einem Sudoku S
        mit allen möglichen Einträgen für jede Position.
        """
        kandidatenListe = [];
        for i in range(9):
            kandidatenListe.append([]);
            for j in range(9):
                if S[i][j]:
#                   Wenn die i.Zeile und j.Spalte besetzt ist
#                   kann nichts mehr eingetragen werden.
                    kandidatenListe[i].append(set())
                else:
#                   Ansonsten ist eine Zahl genau dann Kandidat,
#                   wenn sie noch nicht in dem Block, der Zeile
#                   und der Spalte eingetragen werden kann.
                    kandidatenListe[i].append(BZS[0][i//3*3+j//3] & BZS[1][i] & BZS[2][j])
        return kandidatenListe
    
    
    def BZS_Z_init(S):
        """
        erstellt 2 Hilfs-Listen BZS und Z zu einem Sudoku S
        
        BZS ist eine 3x9 Liste, wobei bei der 1. Dimension 
        BZS[0]=Blöcke
        BZS[1]=Zeilen
        BZS[2]=Spalten  entspricht
        und in der 2. Dimension durchnummeriert wird.
        Bei den Blöcken ist die Reinfolge:  [1 2 3]
                                            [4 5 6]
                                            [7 8 9]
        Als Einträge hat BZS die noch fehlenden Zahlen
        in dem Block/ der Zeile/ der Spalte
        
        Z ist das genaue Gegenstück,
        es ist ein Array der Länge 10, wobei Index 0
        nichts speichert.
        Beim Rest ist der Eintrag eine Menge die jeden
        Tupel (Zeile/Spalte , nr) beinhaltet, in der die 
        jeweilige zum Index gehörende Zahl noch fehlt.
        (Für das Speichern der Blöcke in Z hat sich kein
        Sinnvolle Verwendung gefunden)
        """

        BZS = [[set(range(1, 10)) for nr in range(9) ] for art in range(3)];
        
        Z = ["DUMMY"];
        for k in range(9):
             Z.append(set(it.product(range(1, 3), range(9))))
                
        for i in range(9):
            for j in range(9):
                if S[i][j]:
                    BZS[0][i//3*3+j//3].remove( S[i][j] )
                    BZS[1][i].remove( S[i][j] )
                    BZS[2][j].remove( S[i][j] )
                    Z[S[i][j]].remove( (1, i) )
                    Z[S[i][j]].remove( (2, j) )
        return BZS, Z
    
    
    def zuBearbeiten_init(BZS):
        """
        erstellt zuBearbeiten
        zuBearbeiten ist die Menge aller zahlen und
        Blöcke/Zeilen/Spalten, bei denen es sein könnte, 
        dass man die Zahl in den/r Block/Zeile/Spalte
        eintragen kann.
        Am Anfang ist das jede Zahl in jede(r) Block/Zeile/Spalte
        in der die Zahl noch fehlt.
        Später müssen nur die betrachtet werden, für die sich
        was seit der letzten Betrachtung verändert hat.
        """
        return { (zahl, art, nr) for art in range(3) for nr in range(9) for zahl in BZS[art][nr]}
    
    
    def Spaltenloeser(kandidatenListe, j, kandidat):
        """
        prüft ob der Kandidat in der j. Spalte eingetragen 
        werden kann und gibt ggf die Position zurück
        """
        eintragen_pos=False
        for i in range(9):
            if kandidat in kandidatenListe[i][j]:
        #   wenn die Zahl an dieser Stelle (i,j) in der
        #   Spalte eingetragen werden kann und an keiner 
        #   anderen Stelle, so muss sie hier richtig sein
                if not eintragen_pos:
                    eintragen_pos = (i, j)
                else:
            #   leider kann sie an mind 2 Stellen Eingetragen werden,
            #   also abbrechen
                    return False;
        return eintragen_pos

    
    def Zeilenloeser(kandidatenListe, i, kandidat):
        """
        prüft ob der Kandidat in der i. Zeile eingetragen 
        werden kann und gibt ggf die Position zurück
        """
        eintragen_pos = False
        for j in range(9):
            if kandidat in kandidatenListe[i][j]:
        #   wenn die Zahl an dieser Stelle (i,j) in der
        #   Zeile eingetragen werden kann und an keiner 
        #   anderen Stelle, so muss sie hier richtig sein
                if not eintragen_pos:
                    eintragen_pos = (i, j)
                else:
            #   leider kann sie an mind 2 Stellen Eingetragen werden,
            #   also abbrechen
                    return False;
        return eintragen_pos


    def Blockloeser(kandidatenListe, nr, kandidat):
        """
        prüft ob der Kandidat in dem Block "nr" eingetragen 
        werden kann und gibt ggf die Position zurück
        """
        a, b = nr//3*3, nr%3*3
        eintragen_pos = False
        for i in range(a, a+3):
            for j in range(b, b+3):
                if kandidat in kandidatenListe[i][j]:
            #   wenn die Zahl an dieser Stelle (i,j) in dem
            #   Block eingetragen werden kann und an keiner 
            #   anderen Stelle, so muss sie hier richtig sein
                    if not eintragen_pos:
                        eintragen_pos = (i, j)
                    else:
                #   leider kann sie an mind 2 Stellen Eingetragen werden,
                #   also abbrechen
                        return False;
        return eintragen_pos
    
    
    def copy_2dim(liste):
        """
        tiefes Kopieren einer 2-dimensionalen Liste,
        viel schneller als copy.deepcopy
        """
        return [[ eintrag.copy() for eintrag in zeile] for nr,zeile in enumerate(liste)]
    
    
    def update(zuBearbeiten,BZS,Z,zeile,spalte,zahl):
        """
        aktualisiert die Mengen zuBearbeiten, BZS und Z
        nachdem die Zahl "zahl" an der Stelle "pos" eingetragen wurde
        """
#       Bestimmt Block in der die Zahl eingetragen wurde
        block=zeile//3*3+spalte//3
        
#       entfehrnt die eingetragene Zahl von der Menge 
#       der Zahlen die in B/Z/S noch fehlen
        BZS[0][block].remove(zahl)
        BZS[1][zeile].remove(zahl)
        BZS[2][spalte].remove(zahl)
        
#       entfehrnt B/Z/S von der Menge der Blöcke/Zeilen/Spallten
#       in der die eingetragene Zahl noch fehlt
        Z[zahl].difference_update({(1,zeile),(2,spalte)})
        
#       entfehrnt die eingetragene Zahl aus 
#       den zu bearbeitenden Möglichkeiten
        zuBearbeiten.difference_update({(zahl,0,block),(zahl,1,zeile),(zahl,2,spalte)})
        
#       fügt alle Zahlen die noch in B/Z/S eingetragen werden müssen
#       in die Menge der zu überprüfenden Möglichkeiten ein.
        for zahl_BZS in BZS[0][block]:
            zuBearbeiten.add((zahl_BZS,0,block))
        for zahl_BZS in BZS[1][zeile]:
            zuBearbeiten.add((zahl_BZS,1,zeile))
        for zahl_BZS in BZS[2][spalte]:
            zuBearbeiten.add((zahl_BZS,2,spalte))

            
    def entfehrne_Zahl_aus_kandidatenListe(kandidatenListe,zuBearbeiten,sofortloesen,Z,zeile,spalte,zahl):
        """
        Diese Fkt geht Zeile/Spalte/Block von der "pos" der gerade eingetragenen
        "zahl" durch, um diese aus der "kandidatenListe" zu streichen.
        Ist dies an einer Stelle geschehen, wird geschaut, ob die Zahl
        nun in der Zeile/Spalte/Block dieser Stelle eintragen werden kann,
        indem sie der Menge zuBearbeiten hinzugefügt wird.
        """
#       Blockposition (die Position des oberen linken Feldes des Blockes)
        bl_z=zeile//3*3
        bl_s=spalte//3*3
        
#       Es müssen lediglich die Stellen überprüft werden,
#       in denen die Zahl in der Zeilen/Spalten fehlt
#       ansonsten war die Zahl schon vorher nicht in 
#       der kandidatenListe
        for (art,nr) in Z[zahl]:
            if art==1:
#               Die Eigentliche Arbeit übernimmt entfernen,
#               sie gibt dann zurück, ob die Zahl vorher an
#               der Position eingetragen werden konnte
                if(entfernen(kandidatenListe,sofortloesen,nr,spalte,zahl)):
#                   wenn dem so war, dann sollte die Zeile in der
#                   eingetragen wurde überprüft werden, ob die Zahl
#                   jetzt auch in diese eingetragen werden kann.
                    zuBearbeiten|={(zahl,art,nr)}
                    if nr//3!=zeile//3 :
#                       und wenn das auch noch in einem anderen Block war,
#                       als die eingetragene Zahl, dann sollte der Block auch
#                       nochmal überprüft werden.
                        zuBearbeiten|={(zahl,0,nr//3*3+spalte//3)}
            else:           
                if(entfernen(kandidatenListe,sofortloesen,zeile,nr,zahl)):
#                   wenn dem so war, dann sollte die Spalte in der
#                   eingetragen wurde überprüft werden, ob die Zahl
#                   jetzt auch in diese eingetragen werden kann.
                    zuBearbeiten|={(zahl,art,nr)}
                    if nr//3!=spalte//3 :
#                       und wenn das auch noch in einem anderen Block war,
#                       als die eingetragene Zahl, dann sollte der Block auch
#                       nochmal überprüft werden.
                        zuBearbeiten|={(zahl,0,bl_z+nr//3)}
    
#       In der Zeile und Spalte der eingetragenen Zahl wurde diese 
#       schon aus der kandidatenListe entfehrnt. Es gibt aber noch vier
#       restliche Kästchen in dem Block in dem sie eingetragen wurde:
        for x in range(bl_z,bl_z+3):
            for y in range(bl_s,bl_s+3):
                if(x!=zeile and y!=spalte):
                    if(entfernen(kandidatenListe,sofortloesen,x,y,zahl)):
                        zuBearbeiten|={(zahl,1,x),(zahl,2,y)}
    
    
    def entfernen(kandidatenListe, sofortloesen, zeile, spalte, zahl):
        """
        nimmt >zahl< aus der >kandidatenListe< vom Soduku S 
        an der Stelle pos=(i,j) falls zahl enthalten ist.
        Wenn es danach nur noch eine Möglichkeit gibt,
        wird diese in sofortlösen eingetragen
        """
        
        if zahl in kandidatenListe[zeile][spalte]:
            kandidatenListe[zeile][spalte].remove(zahl)
            if(len(kandidatenListe[zeile][spalte])==1):
                sofortloesen.add((zeile, spalte));
                return False
            return True;
        
        
    def eintragen(S, kandidatenListe, zuBearbeiten, sofortloesen, BZS, Z, zeile, spalte, zahl):
        S[zeile][spalte]=zahl;
        kandidatenListe[zeile][spalte]=set();
        update(zuBearbeiten, BZS, Z, zeile, spalte, zahl);
        entfehrne_Zahl_aus_kandidatenListe(kandidatenListe, zuBearbeiten, sofortloesen, Z, zeile, spalte, zahl);
    
    def raten(S, kandidatenListe, BZS, Z, zeile, spalte):
        for zahl in kandidatenListe[zeile][spalte]:
#           Kopieren, falls man falsch rät
            S_c = [S[i].copy() for i in range(9)]
            kandidatenListe_c = copy_2dim(kandidatenListe)
            BZS_c = copy_2dim(BZS)
            Z_c = ["DUMMY"];
            Z_c.extend([Z[i].copy() for i in range(1,10)])

#           zuBearbeiten und sofortloesen waren leer, ansonsten
#           hätte man nicht raten müssen, desswegen mussten sie
#           nicht kopiert werden
            zuBearbeiten=set()
            sofortloesen=set()

            eintragen(S_copy,
                      kandidatenListe_copy,
                      zuBearbeiten,
                      sofortloesen,
                      BZS_copy,
                      Z_copy,
                      zeile, spalte,
                      zahl);
            ergebnis=loesen(S_copy,
                            kandidatenListe_copy,
                            zuBearbeiten,
                            sofortloesen,
                            BZS_copy,
                            Z_copy);
            if not ergebnis is None:
                return ergebnis
        return
    
    
    def loesen(S,kandidatenListe,zuBearbeiten,sofortloesen,BZS,Z):
        while(True):
            if len(sofortloesen):
                pos = sofortloesen.pop()
                if len(kandidatenListe[pos[0]][pos[1]]):
#                   Wenn 
#                   
                    zahl=kandidatenListe[pos[0]][pos[1]].pop()
                else :
#                   wiederspruch zur Lösbarkeit.
#                   Da die Position in sofortloesen war,
#                   gab es nur eine Zahl die eingetragen werden konnte,
#                   inzwischen kann man keine Zahl mehr eintragen,
#                   und da "sofortloesen" priorität über die anderen
#                   Lösungsstrategien hat, kann die Zahl auch nicht
#                   schon eingetragen worden sein.
#                   Ergo: Hier ist ein freies Feld ohne
#                   anscheinend falsch geraten
#                   abbrechen!
#TODO überprüfen ob tatsächlich alle Möglichkeiten in denen man Falsch geraten hat, an dieser Stelle anspringen
                    return;
            elif len(zuBearbeiten):
#               Hier wird überprüft, ob man durch "scannen" 
#               eine Zahl eintragen kann
                (zahl,art,ort)=zuBearbeiten.pop()
                if(art==0):
                    pos = Blockloeser(kandidatenListe,ort,zahl)
                elif(art==1):
                    pos = Zeilenloeser(kandidatenListe,ort,zahl)
                elif(art==2):
                    pos = Spaltenloeser(kandidatenListe,ort,zahl)
            else:
#               Es kann nichts mehr mit Sicherheit gefunden werden
#               es muss geraten werden;
                break;

#           Wenn eine Position "pos" gefunden wurde,
#           wird die Zahl dort eingetragen
            if pos:
                eintragen(S, kandidatenListe, zuBearbeiten, sofortloesen, BZS, Z, pos[0], pos[1], zahl);
                
#       Ok, der Algorithmus hat alles Eingetragen,
#       was er finden konnte
#       Entweder er ist fertig, oder wir raten.

#       Um effizient zu raten, wird die Stelle mit den wenigsten
#       Möglichkeiten gesucht.
        for mindest in range(2,9):
            for i in range(9):
                for j in range(9):
                    if(len(kandidatenListe[i][j])==mindest):
                        return raten(S,kandidatenListe,BZS,Z,i,j);
        return S
    
    
#   Ab hier geht es los..
#   Alle Hilfslisten initiieren und loslegen ٩(^ᴗ^)۶
    BZS,Z=BZS_Z_init(S)
    kandidatenListe=kandidatenListe_init(S,BZS)
    zuBearbeiten=zuBearbeiten_init(BZS)
    sofortloesen=set();
    return loesen(S,kandidatenListe,zuBearbeiten,sofortloesen,BZS,Z);
Ich habe Python vor einem halben Jahr angefangen und vorher ein Jahr Java gelernt (Uni)
(damit ihr ungefähr meine Vorkenntnisse einschätzen könnt)
Ich Programmiere übrigens über Jupyter, falls das ein Unterschied macht.

ps:
Die Regeln die ich mir durchlesen soll, bevor ich etwas poste, sind irgendwie nicht existenz:
This page does not exist yet. You can create a new empty page, or use one of the page templates.

ich denke aber, ich kann mir ungefähr denken, was drinsteht.
BlackJack

@malin: `sudokuloeser3()` ist zu lang und entweder ist das unsauber oder unnötig die ganzen Funktionen in diese Funktion zu stecken.

Funktioniert das überhaupt? In `raten()` werden zum Beispiel einige Namen die auf `_copy` enden verwendet wo ich keine Definition für finde‽

Es sind so einige Semikolons zu viel und Leerzeichen zu wenig. Nach Kommas und um binäre Operatoren sollte man ein Leerzeichen setzen, damit das lesbarer ist.

Einbuchstabige Namen und kryptische Abkürzungen sollte man vermeiden. Namen komplett in Grossbuchstaben sind per Konvention für Konstanten. Namen ausser für Konstanten und Klassen werden klein_mit_unterstrichen geschrieben. Siehe Style Guide for Python Code.

”Blockkommetare” sollten das Kommentarzeichen entsprechend eingerückt haben. Das ist jetzt gerade ganz konkret ein Problem wenn man die ganzen Funktionen aus der `sudokuloeser3()` heraus kopiert und eine Ebene weniger tief einrückt. Die Kommentare beginnen schon am Zeilenanfang, können also nicht weiter nach links rücken und dann stimmt die Textausrichtung nicht mehr mit dem Code überein. Beispiel nach dem herauslösen einer Funktion:

Code: Alles auswählen

def update(zuBearbeiten, BZS, Z, zeile, spalte, zahl):
    """
    aktualisiert die Mengen zuBearbeiten, BZS und Z
    nachdem die Zahl "zahl" an der Stelle "pos" eingetragen wurde
    """
#       Bestimmt Block in der die Zahl eingetragen wurde
    block = zeile//3*3+spalte//3

#       entfehrnt die eingetragene Zahl von der Menge
#       der Zahlen die in B/Z/S noch fehlen
    BZS[0][block].remove(zahl)
    BZS[1][zeile].remove(zahl)
    BZS[2][spalte].remove(zahl)

#       entfehrnt B/Z/S von der Menge der Blöcke/Zeilen/Spallten
#       in der die eingetragene Zahl noch fehlt
    Z[zahl].difference_update({(1, zeile), (2, spalte)})

#       entfehrnt die eingetragene Zahl aus
#       den zu bearbeitenden Möglichkeiten
    zuBearbeiten.difference_update({(zahl, 0, block), (zahl, 1, zeile), (zahl, 2, spalte)})

#       fügt alle Zahlen die noch in B/Z/S eingetragen werden müssen
#       in die Menge der zu überprüfenden Möglichkeiten ein.
    for zahl_BZS in BZS[0][block]:
        zuBearbeiten.add((zahl_BZS, 0, block))
    for zahl_BZS in BZS[1][zeile]:
        zuBearbeiten.add((zahl_BZS, 1, zeile))
    for zahl_BZS in BZS[2][spalte]:
        zuBearbeiten.add((zahl_BZS, 2, spalte))
Es wird zu viel mit Indexwerten gearbeitet. Zum Beispiel bei BZS das 0 für Blöcke 1 für Zeilen und 2 für Spalten steht ist ”magisch”, muss irgendwo dokumentiert werden, und dann muss man sich das merken oder immer wieder die Dokumentation dafür suchen. Entweder man macht da drei Einzelwerte draus oder man fasst das in einem `collections.namedtuple()` oder einem Wörterbuch zusammen. Dann versteht man jedes mal wenn das verwendet wird was es bedeutet, also ``BZS.bloecke`` statt ``BZS[1]`` (mit einem besseren Namen für `BZS`!).

Auch in Schleifen werden ab und zu Indexwerte verwendet, die gar nicht notwendig wären.

Grunddatentypen haben in Namen eigentlich nichts zu suchen. Den Typ kann man während der Entwicklung schnell mal ändern zu etwas spezialisierterem oder gar etwas selbst geschriebenes, und dann muss man entweder überall die Namen anpassen oder man hat falsche und irreführende Namen im Code.

Im `BZS_Z_init()`-DocString nennst Du eine Liste „Array“, das sollte man auch nicht machen. Arrays gibt es in Python auch, das ist aber ein anderer Datentyp.

`Spaltenloeser()` und `Zeilenloeser()` sehen fast identisch aus. Das sollten keine zwei Funktionen sein.

`eintragen_pos` ist in den Funktionen mal ein Wahrheitswert (aber immer nur `False`, niemals `True`) und mal ein Tupel. Und die Funktionen geben auch entweder `False` (aber nie `True`) oder ein Tupel zurück. Das sollte nicht sein. Als ”nichts”-Wert gibt es `None`. Aber selbst damit sollte man noch überlegen statt eines besonderen Rückgabewertes eine Ausnahme auszulösen, damit der Aufrufer nicht prüfen muss, beziehungsweise die Prüfung nicht vergessen werden kann.

Was soll das `enumerate()` in `copy_2dim()`? `nr` wird nirgends verwendet.

Ein globales ersetzen von 'entfehrn' durch 'entfern' wäre gut. :-)

``if``/``while``/… sind keine Funktionen. Also sollte man sie auch nicht so schreiben als wären es welche. Um die Bedingung gehört keine unnötige Klammer und zwischen Schlüsselwort und Klammer ein Leerzeichen.

Einige Funktionen bekommen zu viele Argumente. Da sollte man überlegen Argumente zu Verbundtypen zusammen zu fassen oder gleich richtig zu Klassen mit entsprechenden Methoden.
Benutzeravatar
kbr
User
Beiträge: 1487
Registriert: Mittwoch 15. Oktober 2008, 09:27

@malin: Vielleicht interessiert Dich auf Deinem Weg zu einem eigenen Sudoku-Löser der folgende Link: http://norvig.com/sudoku.html
malin
User
Beiträge: 11
Registriert: Donnerstag 9. Februar 2017, 14:24

@BlackJack
Vielen Dank, ich werde versuchen das auszubessern,
sudokuloeser3()` ist zu lang und entweder ist das unsauber oder unnötig die ganzen Funktionen in diese Funktion zu stecken.
Naja alle Funktionen gehören zum sudokuloeser3, wozu gibt es Unterfunktionen, wenn nicht, um Zugehörigkeit zu verdeutlichen?
Dann sieht auch jeder, welche die Funktion ist, die nach außen aufgerufen wird.
Funktioniert das überhaupt? In `raten()` werden zum Beispiel einige Namen die auf `_copy` enden verwendet wo ich keine Definition für finde‽
Das ist mir jetzt ganz peinlich :oops:.
Ich war mir nicht sicher ob ich die Kopien mit _copy enden lassen sollte oder mit _c
jetzt habe ich mitten drin aufgehört.
Das war aber der einzige "richtige" Fehler, ich habe die Funktion jetzt nochmal mit paar Matrizen durch getestet.
Es sind so einige Semikolons zu viel und Leerzeichen zu wenig. Nach Kommas und um binäre Operatoren sollte man ein Leerzeichen setzen, damit das lesbarer ist.
Das mit dem Leerzeichen habe ich mir schon gedacht, und auch an vielen Stellen hinzugefügt. Das werde ich auf jeden Fall zu ende führen.
Semikolons sind wohl eine schlechte Angewohnheit aus Java, werde ich entfernen. (haben die einen anderen Sinn, als die Ausgabe zu unterdrücken?)
Einbuchstabige Namen und kryptische Abkürzungen sollte man vermeiden. Namen komplett in Grossbuchstaben sind per Konvention für Konstanten. [...]
Es wird zu viel mit Indexwerten gearbeitet. Zum Beispiel bei BZS das 0 für Blöcke 1 für Zeilen und 2 für Spalten steht ist ”magisch”, muss irgendwo dokumentiert werden [...] Entweder man macht da drei Einzelwerte draus oder man fasst das in einem `collections.namedtuple()` oder einem Wörterbuch zusammen. [...] (mit einem besseren Namen für `BZS`!). [...]
Einige Funktionen bekommen zu viele Argumente. Da sollte man überlegen Argumente zu Verbundtypen zusammen zu fassen oder gleich richtig zu Klassen mit entsprechenden Methoden.
Ok, die Namens-Konventionen sind auch in Java so, da kann ich mich nicht rausreden, werde sie überarbeiten.
(Ist Camelcase tatsächlich so verpönt in Python?)
Grund für diese Unsauberkeit war wohl das Problem das am Ende angesprochen wurde.
Zu viele Argumente, dadurch werden ansonsten Funktionen-aufrufe zu lang.
3 Einzelwerte für BZS würde das noch verschlimmern, das will ich vermeiden.
Was sind denn Verbundstypen? und was collections.namedtuple() ?
Meinst du damit die Klassen?
Ist das üblich Klassen nur für Zusammenfassung von Variablen zu benutzen?
Gut, Dictionaries würden auch gehen, aber ich würde da die Klassen-Variante bevorzugen.

Und zur speziellen Namensgebung von BZS:
gut ich habe den Namen genau deswegen so gewählt, damit man die Reihenfolge nicht vergessen kann Blöcke/Zeilen/Spalten
Aber der Name ist natürlich Mist, nur fällt mir absolut nicht ein außer vielleicht:
auflistung_von_allen_zahlen_die_in_dem_block_die_zeilen_oder_der_spalte_noch_fehlen
Und das ist dann doch wieder zu lang.
Ich hoffe ich komme bald auf eine gute Lösung :|
”Blockkommetare” sollten das Kommentarzeichen entsprechend eingerückt haben.
ok, ändere ich
Auch in Schleifen werden ab und zu Indexwerte verwendet, die gar nicht notwendig wären.
wo meinst du das?
Grunddatentypen haben in Namen eigentlich nichts zu suchen
Ich nehme an, du meinst die "kandidatenListe".
Das ist ein fester Begriff unter Sudokulösern (jedenfalls bin ich darauf an verschiedenen Stellen im Internet gestoßen).
Ich würde den Namen auch nicht ändern, wenn ich mich dazu entschließen würde, es als np.array oder Dictionary
zu implementieren.
Im `BZS_Z_init()`-DocString nennst Du eine Liste „Array“
ok ertappt. Es ist genau das passiert was du gerade gesagt hast, es war am Anfang ein np.array
bis ich gemerkt habe, dass ich damit nicht rechnen will und Listen effizienter sind.
Ich habe vergessen das um zu ändern.
Ein globales ersetzen von 'entfehrn' durch 'entfern' wäre gut.
Schande über mich :oops:
``if``/``while``/… sind keine Funktionen. Also sollte man sie auch nicht so schreiben als wären es welche. Um die Bedingung gehört keine unnötige Klammer und zwischen Schlüsselwort und Klammer ein Leerzeichen.
Altlasten von Java, werden entfernt.

Aber erstmal:
VIELEN, VIELEN DANK (keine Sorge, mein Dank ist total konstant)
Du hast dir viel Zeit genommen und an vielen Stellen sehr genau hin geschaut.
Ich werde daran arbeiten ein weniger schlechter Programmierer zu werden. :wink:
malin
User
Beiträge: 11
Registriert: Donnerstag 9. Februar 2017, 14:24

@kbr
Danke, das werde ich mir anschauen sobald ich fertig bin mit den Ausbesserungen.
Sirius3
User
Beiträge: 17749
Registriert: Sonntag 21. Oktober 2012, 17:20

@malin: verschachtelte Funktionen sind nicht einzeln testbar und auch nicht wiederverwendbar. Daher, sind sie zu vermeiden.
BlackJack

@malin: Funktionen sind dazu da um andere Funktionen zusammen zu fassen. Wie willst Du denn bei der Fehlersuche oder in Unittests diese inneren Funktionen einzeln testen? Wenn Du Funktionen zusammenfassen möchtest um beispielsweise verschiedene Sudokulöser zu trennen, dann steck die in einzelne Module.

Innere Funktionen gibt es für Closures. Wovon Du allerdings keinen gebrauch machst, was den Code unverständlicher macht, denn danach wird ein Leser suchen und nichts in der Richtung finden. Sofern er wirklich diese ganze lange Funktion danach untersucht. Das wäre wegen der Länge auch furchtbar unübersichtlich, denn auch wenn man Closures verwendet, sollte die äussere Funktion in der Regel so maximal 25 bis 50 Zeilen lang sein. Danach versteht das keiner mehr.

Semikolons unterdrücken keine Ausgabe. Die trennen Anweisungen, machen also wirklich nur Sinn wenn auf beiden Seiten eine nicht-leere Anweisung steht. Und üblich ist es nur eine Anweisung pro Zeile zu schreiben. Da gibt es ein paar wenige Ausnahmen, die aber wirklich selten sind.

Gegenfrage: Wie beliebt sind kleinbuchstaben_mit_unterstrichen bei Java? ;-)

Verbundtypen sind Datentypen die mehrere Werte zu einem zusammenfassen. Zum Beispiel Typen die mit `collections.namedtuple` erzeugt wurden. Oder Klassen. Klassen nur zum zusammenfassen von Werten zu verwenden ist selten, aber das bleibt üblicherweise auch nicht dabei. In der Regel findet man dann schon sinnvolle Operationen die man als Methoden implementieren kann. Manchmal auch Verhalten das man über die ”magischen” Methoden erreicht, also die mit den doppelten Unterstrichen vorne und hinten.

Weniger Indexzugriffe kann man beispielsweise in `kandidatenListe_init()` haben wenn man keine leere Liste an die Ergebnisliste anhängt, sondern die erst füllt und am Ende hinzufügt (Ausgangspunkt, ohne Kommentare und DocString):

Code: Alles auswählen

def kandidatenListe_init(S, BZS):
    kandidatenListe = []
    for i in range(9):
        kandidatenListe.append([])
        for j in range(9):
            if S[i][j]:
                kandidatenListe[i].append(set())
            else:
                kandidatenListe[i].append(BZS[0][i//3*3+j//3] & BZS[1][i] & BZS[2][j])
    return kandidatenListe
Erst einmal so viele wie möglich von den Indexzugriffen beseitigt:

Code: Alles auswählen

def kandidaten_init(S, BZS):
    bloecke, zeilen, spalten = BZS
    kandidaten = []
    for i, (s_zeile, zeile) in enumerate(zip(S, zeilen)):
        zeile = []
        for j, (s_zahl, spalte) in enumerate(zip(s_zeile, spalten)):
            zeile.append(
                set() if s_zahl else bloecke[i//3*3+j//3] & zeile & spalte
            )
        kandidaten.append(zeile)
    return kandidaten
Dann die Schleifen in „list comprehensions“ umgeschrieben:

Code: Alles auswählen

def kandidaten_init(S, BZS):
    bloecke, zeilen, spalten = BZS
    return [
        [
            (set() if s_zahl else bloecke[i//3*3+j//3] & zeile & spalte)
            for j, (s_zahl, spalte) in enumerate(zip(s_zeile, spalten))    
        ]
        for i, (s_zeile, zeile) in enumerate(zip(S, zeilen))
    ]
Das sieht alles immer noch recht komplex aus. IMHO weil die Datenstrukturen suboptimal sind. Das man hier so viel zusammen `zip()`\en muss, liegt daran das eigentlich zusammengehörende Information auf verschiedene Datenstrukturen verteilt sind. Eigentlich möchte man für jede Zelle im Sudoku wissen ob und welche Zahl sie enthält, welche Zahlen in der Zeile, in der Spalte, und in der Box zu der die Zelle gehört, schon verwendet wurden. Oder noch nicht verwendet wurden, je nach dem mit welcher Variante sich besser arbeiten lässt. Da hätte man dann `Cell`- oder `Square`-Objekte die je ein Attribut für diese Dinge haben, also eine Zahl oder `None` für die Zahl und jeweils `set()`\s für Zeile, Spalte, und Block. Dabei teilen sich dann alle Zellen jeder Zeile ein `set()` für die Zeileninformation und die Zellen aller Spalten ein `set()` für die Spalteninformation, und die Zellen in jedem Block ein `set()` für die Blockinformation.

Da ergeben sich dann beispielsweise auch gleich zwei Methoden: Zahl setzen und löschen, was die entsprechenden verbundenen `set()`\s aktualisiert. Und Abfragen der Kandidaten, die sich ja aus den `set()`\s berechnen lassen.

Zum Initialisieren würde ich auch nichts so spezielles schreiben, sondern einfach ein leeres Sudoku-Feld in dieser Form erstellen und dann die Zahlen aus einer 2D-Liste dort ganz normal eintragen.


Noch ein Beispiel für unnötigen Indexzugriff:

Code: Alles auswählen

S_c = [S[i].copy() for i in range(9)]
# ->
S_c = [s.copy() for s in S]
Bei den Listen mit dem "DUMMY" als erstem Element würde ich diesen "DUMMY" loswerden. In der Informatik fängt man halt bei 0 an zu zählen. Wenn der Benutzer 1 haben will, dann behandelt man das sauber bei der Ein- und Ausgabe und spart sich intern so komische Sonderfälle wie erste Elemente die nicht verwendet werden, aber immer eine Sonderbehandlung im Code benötigen.

Hierbei könnte man die Position gleich entpacken:

Code: Alles auswählen

                pos = sofortloesen.pop()
                if len(kandidatenListe[pos[0]][pos[1]]):
                    zahl=kandidatenListe[pos[0]][pos[1]].pop()

                # ->
                
                zeile, spalte = sofortloesen.pop()
                if kandidatenListe[zeile][spalte]:
                    zahl=kandidatenListe[zeile][spalte].pop()
Wobei man auch das `pop()` einfach durchführen könnte und auf die Ausnahme reagieren könnte, die laut Kommentar sowieso nie auftreten dürfte. Falls sie es doch tut, stimmt sowieso etwas nicht, also braucht man sie auch nicht behandeln.

Hier haben wir einen Fall für *mehr* Indexzugriffe (ja, das gibt's auch :-)):

Code: Alles auswählen

                (zahl,art,ort)=zuBearbeiten.pop()
                if(art==0):
                    pos = Blockloeser(kandidatenListe,ort,zahl)
                elif(art==1):
                    pos = Zeilenloeser(kandidatenListe,ort,zahl)
                elif(art==2):
                    pos = Spaltenloeser(kandidatenListe,ort,zahl)

                # ->

                zahl, art, ort = zuBearbeiten.pop()
                loeser = [Blockloeser, Zeilenloeser, Spaltenloeser]
                pos = loeser[art](kandidatenListe, ort, zahl)
Da `loeser` ”statisch” ist, könnte man das auch auf Modulebene als Konstante erstellen, oder zumindest am Anfang in der Funktion, vor der Schleife. Alternativ könnte man statt der Zahl `art` auch gleich die passende Funktion als zweites Element in die Tupel packen. Funktionen sind in Python ja Werte wie alle anderen auch. Vergisst man als Java-Programmierer gerne mal, obwohl es bei Java ja auch in die Richtung von mehr funktionaler Programmierung geht.
malin
User
Beiträge: 11
Registriert: Donnerstag 9. Februar 2017, 14:24

ach ja, zu:
`Spaltenloeser()` und `Zeilenloeser()` sehen fast identisch aus. Das sollten keine zwei Funktionen sein.
Daran habe ich auch schon gedacht, aber wie?

Code: Alles auswählen

def scanner(kandidatenliste, art, nr, kandidat):
    """
    prüft ob der Kandidat in der j. Spalte eingetragen 
    werden kann und gibt ggf die Position zurück
    """
    eintragen_pos = None
    for feld in range(9):
        if art == "block":
            i = nr//3 *3 + feld//3
            j = nr%3 *3 + feld%3
        elif art == "zeile":
            i, j = nr, feld
        else:
            i, j = feld, nr
        if kandidat in kandidatenliste[i][j]:
            # wenn die Zahl an dieser Stelle (i,j) in der
            # Spalte eingetragen werden kann und an keiner 
            # anderen Stelle, so muss sie hier richtig sein
            if not eintragen_pos:
                eintragen_pos = (i, j)
            else:
                # leider kann sie an mind 2 Stellen Eingetragen werden,
                # also abbrechen
                return None;
    return eintragen_pos
Das überzeugt mich nicht 100%
So habe ich zwar alle 3 Löser zusammen geschmolzen,
aber dafür wird das ganze irgendwie unhandlicher.
Dieser Codeblock wird auch mit am häufigsten von allen ausgeführt (1497 mal bei dem schwersten Sudoku)
und verbraucht mit 1/4 der gesammten Zeit am meisten von allen Funktionen.
dass er jetzt in der Schleife 3 Fallunterscheidungen hat + Rechnungen (beim 1. Fall)
verlangsamt das ganze schon ein bisschen, (ist aber noch im erträglichen Bereich)
Die Variante?
Oder andere Ideen?
malin
User
Beiträge: 11
Registriert: Donnerstag 9. Februar 2017, 14:24

Zu dem Sofortentpacken durch .pop()
Mit try und except :
Das habe ich zuerst gemacht,
habe mich aber letztlich um entschieden,
weil es durch exceptions nicht kürzer wird,
sondern nur eine Einrückungsebene tiefer,
weil es auf der Ebene von exceptions kein elif gibt.
(Und der Fall kann auftreten, da ich ja zwischendurch raten muss,
ich muss dann nur eine Rekonstruktion höher was anderes raten)

Code: Alles auswählen

if len(sofortloesen):
    pos = sofortloesen.pop()
    if len(kandidatenListe[pos[0]][pos[1]]):  
        zahl=kandidatenListe[pos[0]][pos[1]].pop()
    else :
        return;
elif len(zuBearbeiten):
    (zahl,art,ort)=zuBearbeiten.pop()
    pos = scanner(kandidatenListe,art,ort,zahl)
else:
    break
if pos:
    eintragen(S, kandidatenListe, zuBearbeiten, sofortloesen, BZS, Z, pos[0], pos[1], zahl)

########################################################################

try:
    pos = sofortloesen.pop()
    try:
        zahl=kandidatenListe[pos[0]][pos[1]].pop()
    except KeyError:
        return;
except KeyError:
    try:
        (zahl,art,ort)=zuBearbeiten.pop()
        pos = scanner(kandidatenListe,art,ort,zahl)
    except KeyError:
        break
if pos:
    eintragen(S, kandidatenListe, zuBearbeiten, sofortloesen, BZS, Z, pos[0], pos[1], zahl)
malin
User
Beiträge: 11
Registriert: Donnerstag 9. Februar 2017, 14:24

@BlackJack
(Ich hoffe Doppelpostings gehört nicht zu den Regeln, die nicht angezeigt werden können >_< und ich somit gerade breche)
Zu Closures:
das meint doch "nonlocal"-Variablen, oder?
Damit kann ich natürlich ohne weiteres Probleme wie zuviele übergaben an Funktionen beheben.
Alle Listen könnten als nonlocal betietelt werden,
da es jede nur einmal im Programm als eindeutiges "objekt" existiert.
(jedenfalls nachdem ich die Funktion von Kopie und nicht Kopie beim raten vertauschen würde)
Aber ich dachte nonlocal wäre der kleine Bruder von global
und man würde auf der Stelle gesteinigt werden,
wenn man so etwas benutzt :D
Benutzeravatar
pillmuncher
User
Beiträge: 1484
Registriert: Samstag 21. März 2009, 22:59
Wohnort: Pfaffenwinkel

malin hat geschrieben:Zu Closures:
das meint doch "nonlocal"-Variablen, oder?
Nein. Schau mal:

Code: Alles auswählen

>>> def adder(x):
...     def add(y):
...         return x + y
...     return add
... 
>>> add3 = adder(3)
>>> add3(4)
7
>>> add5 = adder(5)
>>> add5(4)
9
add ist hier eine Closure. Closure heißt auf deutsch Ab- oder Einschluss. Die Idee ist, dass eine Funktion über dem sie umgebenden Namesraum abgeschlossen ist bzw. diesen mit einschließt. Siehe auch den Artikel auf Wikipedia.
In specifications, Murphy's Law supersedes Ohm's.
BlackJack

@malin: Was Du beschreibst mit ``nonlocal`` wäre in der Tat nicht wirklich schön, aus dem Grund den Sirius3 und ich ja schon genannt haben: die lokalen Funktionen sind von aussen nicht testbar (oder wiederverwendbar). Wenn man immer wieder den gleichen Satz an Argumenten an eine Gruppe von Funktionen übergibt, ist es in der Regel Zeit sich Gedanken darüber zu machen ob und wie man die Argumente und die Funktionen zu Klassen und Methoden zusammenfasst.
BlackJack

Ha, ich wusste ich hatte da schon mal drüber nachgedacht und auch einen Löser produziert. Das ist schon mehr als 10 Jahre her: viewtopic.php?p=19685#p19685

Da sind die Mengen für Zeilen, Spalten, Blöcke, und das (teilweise) ausgefüllte Sudoku zu einer Klasse zusammengefasst.
Antworten