Nichts besonderes, es geht vor allem um Verbesserungsvorschläge zum Code an sich...
... Kritik erwünscht - macht mich fertig
http://paste.pocoo.org/show/140802/
Sudokus lösen...
Ein kleines Tool um Sudokus zu lösen.
Nichts besonderes, es geht vor allem um Verbesserungsvorschläge zum Code an sich...
... Kritik erwünscht - macht mich fertig
http://paste.pocoo.org/show/140802/
Nichts besonderes, es geht vor allem um Verbesserungsvorschläge zum Code an sich...
... Kritik erwünscht - macht mich fertig
http://paste.pocoo.org/show/140802/
Zuletzt geändert von Snoda am Montag 21. September 2009, 14:15, insgesamt 2-mal geändert.
Hello world!
Ich würde (ohne mir die genaue Funktionsweise deines Codes zu kennen) erstmal die Struktur des Programms ändern.
Erstmal würde ich für das Spielfeld eine eigene Klasse anlegen (vllt. auch gar nicht nötig), die dann von "list" erbt oder einfach nur ein "list" ist. Dann würde ich auf das Spielfeld so zugreifen "feld[pos_x][pos_y]". Und wenn ein Feld leer ist, würde ich None und nicht 0 verwenden. Für was die Methode setfield ist, verstehe ich auch nicht. Das würde ich mit __setitem__ lösen und evtl. überprüfen, ob diese Eingabe fehlerhaft ist und dementsprechend eine Exception raisen.
Für mehr Feedback fehlt mir gerade die Zeit.
Erstmal würde ich für das Spielfeld eine eigene Klasse anlegen (vllt. auch gar nicht nötig), die dann von "list" erbt oder einfach nur ein "list" ist. Dann würde ich auf das Spielfeld so zugreifen "feld[pos_x][pos_y]". Und wenn ein Feld leer ist, würde ich None und nicht 0 verwenden. Für was die Methode setfield ist, verstehe ich auch nicht. Das würde ich mit __setitem__ lösen und evtl. überprüfen, ob diese Eingabe fehlerhaft ist und dementsprechend eine Exception raisen.
Für mehr Feedback fehlt mir gerade die Zeit.
Für Typprüfungen sollte man `isinstance()` statt `type()` verwenden. Überhaupt programmiert man in Sprachen wie Python ducktyping-freundlich, d.h. man kommt möglichst ohne Typprüfungen aus. Ich persönlich hätte an der Stelle wohl einfach den Slice-Part in einen `try:`-Block geschrieben und wäre bei einem `AttributeError` davon ausgegangen, dass eine Zahl übergeben wurde. Ein Cast mittels `int(number)` hätte mir das notfalls garantiert.
Überhaupt finde ich deinen ganzes System etwas merkwürdig. Slice hätte ich gar nicht so umgeschrieben, sondern etwas in dieser Art gemacht: `make_game(fields[x][y], fields[x][y])`, wobei die Felder halt die Start- und Endposition sind, also Feld oben links bis Feld unten rechts. Davon ab finde ich es auch ein bißchen verwirrend, dass sozusagen Reihe 3 als Listenindex umgedacht werden muss und daher der Index 2 ist. Deshalb hatte ich das Beispiel in deiner Erklärung auch erst beim zweiten Lesen verstanden. Die Frage ist halt, ob eine Liste wirklich die richtige Repräsentation für ein Spielfeld ist...
Überhaupt finde ich deinen ganzes System etwas merkwürdig. Slice hätte ich gar nicht so umgeschrieben, sondern etwas in dieser Art gemacht: `make_game(fields[x][y], fields[x][y])`, wobei die Felder halt die Start- und Endposition sind, also Feld oben links bis Feld unten rechts. Davon ab finde ich es auch ein bißchen verwirrend, dass sozusagen Reihe 3 als Listenindex umgedacht werden muss und daher der Index 2 ist. Deshalb hatte ich das Beispiel in deiner Erklärung auch erst beim zweiten Lesen verstanden. Die Frage ist halt, ob eine Liste wirklich die richtige Repräsentation für ein Spielfeld ist...
Danke, genau so etwas in der Art wollte ich hören, auch wenn ich es noch nicht ganz verstehe 
Ich habe oft das Gefühl dass ich alles zu umständlich mache, und verliere dabei schon in einfachen Dingen die Übersicht. Daran sind alle großen Projekte bislang gescheitert. Ich werde jetzt mal versuchen die angesprochenen Sachen zu verbessern...
€: Es geht also wenn ich das richtig sehe vor allem um meine Klasse "Slist" die anders gestaltet werden sollte?
Ich habe oft das Gefühl dass ich alles zu umständlich mache, und verliere dabei schon in einfachen Dingen die Übersicht. Daran sind alle großen Projekte bislang gescheitert. Ich werde jetzt mal versuchen die angesprochenen Sachen zu verbessern...
€: Es geht also wenn ich das richtig sehe vor allem um meine Klasse "Slist" die anders gestaltet werden sollte?
Hello world!
-
BlackJack
@Snoda: "Ready" heisst auf deutsch "fertig" im Sinne von "bereit". "Ready" ist man vor einer Aufgabe und nicht danach. "Fertig" im Sinne von "Aufgabe ist fertig" heisst "Finished".
`Slist` würde ich nicht von `list` ableiten. Das Objekt hat dadurch einen Haufen Methoden, die keinen Sinn machen, zum Beispiel `sort()`. Was soll der Name bedeuten? Wenn man das fragen muss, ist das oft ein Zeichen, dass er nicht so gut gewählt ist.
Veränderbare Objekte als Defaultwerte für Argumente sind keine gute Idee, weil Defaultwerte nur *einmal* bei der Definition der Funktion ausgewertet werden. Und das `Slist.__init__()` in `data` Zeichenketten mit Ziffern entgegennimmt, wo die Klasse doch eigentlich auf Zahlen operiert, ist etwas "asymmetrisch". Dazu dann noch eine Ausnahme die behauptet, dass nur 1-9 erlaubt sind, wo doch ganz offensichtlich auch 0 geht. Die beiden Schleifen sollten besser von 0 bis 8 laufen und statt beim Index anzupassen, sollte man das wenn schon besser beim Aufruf von `Field()` machen. Statt `xrange()` und der Indexerei auf `data` wäre hier `enumerate()` "pythonischer".
Man sollte sich bei Computern angewöhnen, dass die Zählung bei 0 anfängt und gegebenenfalls nur an der Schnittstelle zum Benutzer den Index für die Ein- und Ausgabe anpassen. In der Programmlogik selbst ist man sonst ständig mit +1 und -1 im Gange und handelt sich subtile Fehler ein, wenn man nicht aufpasst. Es macht das Programm auf jeden Fall etwas komplizierter.
Die Zeilenfortsetungszeichen bei `__repr__()` und `__str__()` in `Slist` sind überflüssig. "Logische" Zeilen enden erst, wenn alle geöffneten Klammern auch wieder geschlossen sind. An der Stelle erkennt Python also ohne weitere Hinweise, dass der Ausdruck noch nicht zuende sein kann.
So etwas wie ``"%s" % str(obj)`` ist übrigens zuviel des Guten. ``"%s" % obj`` bedeutet schon, dass `obj` in eine Zeichenkette umgewandelt werden muss, um an der Stelle eingesetzt zu werden. Das muss man vorher nicht schon mit `str()` machen.
Namen die mit Grossbuchstaben beginnen, sind per Konvention für Klassen vorbehalten. Ausnahme sind Namen, bei denen alle Buchstaben gross geschrieben sind, die sind üblich für Konstanten. Und `S` ist ein zu kurzer und nichtssagender Name. "Possibilities" schreibt man mit zwei "s".
Bei `Field.value` würde sich ein `property()` anbieten. Und bitte entscheide Dich mal, ob Du englische oder deutsche Namen vergibst.
Die "magischen" Methoden mit den Unterstrichen sollte man nur direkt aufrufen, wenn es keine andere Möglichkeit gibt. Also ``str(self.value)`` statt ``self.value.__str__()``. Die Fallunterscheidung in `Field.__repr__()` ist eigentlich überflüssig. Wenn der erste Zweig greift, hätte der zweite doch genau das gleiche Ergebnis gebracht.
``__str__ = __repr__`` ist überflüssig. `object.__str__()` ruft schon von sich aus ``repr(self)`` auf.
"row" und "line" heisst beides "Zeile". Wenn man Zeile und Spalte in Englisch ausdrücken will, dann redet man von "row" und "column". Beim Namen `getquad()` würde man eher erwarten, dass ein "Viertel" zurückgegeben wird.
`Field._update_posibilities()` ist nicht besonders effizient und IMHO auch nicht so logisch aufgebaut, wie es sein könnte. `pos` und `a` sind schlechte Namen. Wenn man schon mit `set()`\s arbeitet, sollte man auch ausnutzen, dass man damit "rechnen" kann. Eine Methode, die einen Wert zurückgibt, sollte das a) in jedem Zweig explizit tun, und nicht einfach in das implizite `None` am Ende laufen dürfen, und sollte b) möglichst immer Objekte vom gleichen "Duck-Typ" liefern und nicht `True` und `None`, was hier wahrscheinlich vom Aufrufer als "Falsch" interpretiert wird.
Du kennzeichnest die Methode durch den führenden Unterstrich als "intern", rufst sie dann aber von einer anderen Klasse aus auf!?
Bei der doch leicht komplizierten Implementierung von `SList.__getitem__()` ist mir nicht so wirklich klar, was ein Aufruf mit ``[1:9,1:9]`` genau liefert. It Dir das klar? Ich meine bis zu welcher Ebene da kopiert wird, und ob das in `Sudoku.update()` überhaupt Sinn macht!? Warum ist `changed` ein Attribut und kein lokaler Name?
Methoden, die einen Wahrheitswert zurückgeben werden üblicherweise mit dem Präfix `has_` oder `is_` oder so ähnlich benannt, damit man schon am Namen sieht, dass sie eine Bedingung prüfen. Allerdings ist das bei `ready()` eigentlich gar nicht nötig, weil die Methode gar keinen Rückgabewert braucht. Das Ende wird ja durch eine Ausnahme angezeigt. Diese syntaktische Form eine Ausnahme auszulösen, sollte man übrigens nicht mehr verwenden.
`get_copy()` ist ziemlich überflüssig. `N` ist ein schlechter Name und auch überflüssig. Eine Kopie von `self.Spiel` zu machen um ein neues `Sudoku`-Exemplar zu erstellen, scheint mir auch überflüssig -- warum nicht gleich eine Kopie von `self` machen!? Und dann ist diese ganze Methode nur noch ``return deepcopy(self)`` und es stellt sich die Frage, ob das echt eine eigene Methode sein muss.
`challange()` ist falsch geschrieben. Du solltest echt einen Spellchecker über Dein Programm laufen lassen.
`N` ist wieder, oder immer noch, ein schlechter Name.
Die erste Zeile der Ausnahmebehandlung in `guess()` greift IMHO viel zu tief in die Datenstruktur ein. Der Sudoku-Löser sollte nicht den internen Aufbau von einzelnen Zellen kennen müssen. Das sollte man an die Zelle weiterdelegieren und die Zelle a.k.a. `Field` sollte auch die Ausnahme auslösen, wenn ihr die Möglichkeiten ausgegangen sind.
Was zum Henker ist `isod` für ein Name!?
Die Namensgebung der Klassen ist unglücklich. `SList` scheint eigentlich das `Sudoku` zu repräsentieren, während `Sudoku` von der Bedeutung eher `SudokuSolver` wäre. Man könnte auch die `SList` in `Field` umbenennen und das jetzige `Field` in `Cell` oder so ähnlich.
Importe sollte man nicht mitten im Modul verstecken sondern an den Anfang schreiben, damit man leicht sehen kann, wovon ein Modul abhängt.
Die `main()` ist wieder voller schlechter Namen. `slist` für etwas was gar keine Liste ist!? `a`, `b`, `s`? Solche Namen sind in der abgeschlossenen, begrenzten "Umgebung" einer "list comprehension" oder eines Generatorausdrucks in Ordnung, aber nicht als Namen mit einem grösseren "Lebensraum". Spezielle Rückgabewerte für Fehler sind schlechter Stil. Für so etwas wurden Ausnahmen erfunden. `get_todo()` sollte immer einen Wert liefern, oder halt eine Ausnahme auslösen, wenn das nicht geht.
Ich überlege gerade, wie man bei `get_todo()` ein `readout` hinbekommt, dass nicht die Länge 9 hat und dieser Test tatsächlich erreicht wird. Die Rückgabewerte sind abenteuerlich. Der Aufrufer hat das Dateiobjekt, kann sich also auch selber den Namen davon holen.
Mehr Leerzeichen wären schön. Der Style-Guide empfiehlt welche um Operatoren und Gleichheitszeichen (ausgenommen: Defaultargumente in Funktionsdefinitionen) und nach Kommas. Und bitte keine "Blöcke" in eine Zeile quetschen. Es ist einfach leichter lesbar, wenn da ein Zeilenumbruch steht, und entsprechend eingerückt ist.
`Slist` würde ich nicht von `list` ableiten. Das Objekt hat dadurch einen Haufen Methoden, die keinen Sinn machen, zum Beispiel `sort()`. Was soll der Name bedeuten? Wenn man das fragen muss, ist das oft ein Zeichen, dass er nicht so gut gewählt ist.
Veränderbare Objekte als Defaultwerte für Argumente sind keine gute Idee, weil Defaultwerte nur *einmal* bei der Definition der Funktion ausgewertet werden. Und das `Slist.__init__()` in `data` Zeichenketten mit Ziffern entgegennimmt, wo die Klasse doch eigentlich auf Zahlen operiert, ist etwas "asymmetrisch". Dazu dann noch eine Ausnahme die behauptet, dass nur 1-9 erlaubt sind, wo doch ganz offensichtlich auch 0 geht. Die beiden Schleifen sollten besser von 0 bis 8 laufen und statt beim Index anzupassen, sollte man das wenn schon besser beim Aufruf von `Field()` machen. Statt `xrange()` und der Indexerei auf `data` wäre hier `enumerate()` "pythonischer".
Man sollte sich bei Computern angewöhnen, dass die Zählung bei 0 anfängt und gegebenenfalls nur an der Schnittstelle zum Benutzer den Index für die Ein- und Ausgabe anpassen. In der Programmlogik selbst ist man sonst ständig mit +1 und -1 im Gange und handelt sich subtile Fehler ein, wenn man nicht aufpasst. Es macht das Programm auf jeden Fall etwas komplizierter.
Die Zeilenfortsetungszeichen bei `__repr__()` und `__str__()` in `Slist` sind überflüssig. "Logische" Zeilen enden erst, wenn alle geöffneten Klammern auch wieder geschlossen sind. An der Stelle erkennt Python also ohne weitere Hinweise, dass der Ausdruck noch nicht zuende sein kann.
So etwas wie ``"%s" % str(obj)`` ist übrigens zuviel des Guten. ``"%s" % obj`` bedeutet schon, dass `obj` in eine Zeichenkette umgewandelt werden muss, um an der Stelle eingesetzt zu werden. Das muss man vorher nicht schon mit `str()` machen.
Namen die mit Grossbuchstaben beginnen, sind per Konvention für Klassen vorbehalten. Ausnahme sind Namen, bei denen alle Buchstaben gross geschrieben sind, die sind üblich für Konstanten. Und `S` ist ein zu kurzer und nichtssagender Name. "Possibilities" schreibt man mit zwei "s".
Bei `Field.value` würde sich ein `property()` anbieten. Und bitte entscheide Dich mal, ob Du englische oder deutsche Namen vergibst.
Die "magischen" Methoden mit den Unterstrichen sollte man nur direkt aufrufen, wenn es keine andere Möglichkeit gibt. Also ``str(self.value)`` statt ``self.value.__str__()``. Die Fallunterscheidung in `Field.__repr__()` ist eigentlich überflüssig. Wenn der erste Zweig greift, hätte der zweite doch genau das gleiche Ergebnis gebracht.
``__str__ = __repr__`` ist überflüssig. `object.__str__()` ruft schon von sich aus ``repr(self)`` auf.
"row" und "line" heisst beides "Zeile". Wenn man Zeile und Spalte in Englisch ausdrücken will, dann redet man von "row" und "column". Beim Namen `getquad()` würde man eher erwarten, dass ein "Viertel" zurückgegeben wird.
`Field._update_posibilities()` ist nicht besonders effizient und IMHO auch nicht so logisch aufgebaut, wie es sein könnte. `pos` und `a` sind schlechte Namen. Wenn man schon mit `set()`\s arbeitet, sollte man auch ausnutzen, dass man damit "rechnen" kann. Eine Methode, die einen Wert zurückgibt, sollte das a) in jedem Zweig explizit tun, und nicht einfach in das implizite `None` am Ende laufen dürfen, und sollte b) möglichst immer Objekte vom gleichen "Duck-Typ" liefern und nicht `True` und `None`, was hier wahrscheinlich vom Aufrufer als "Falsch" interpretiert wird.
Du kennzeichnest die Methode durch den führenden Unterstrich als "intern", rufst sie dann aber von einer anderen Klasse aus auf!?
Bei der doch leicht komplizierten Implementierung von `SList.__getitem__()` ist mir nicht so wirklich klar, was ein Aufruf mit ``[1:9,1:9]`` genau liefert. It Dir das klar? Ich meine bis zu welcher Ebene da kopiert wird, und ob das in `Sudoku.update()` überhaupt Sinn macht!? Warum ist `changed` ein Attribut und kein lokaler Name?
Methoden, die einen Wahrheitswert zurückgeben werden üblicherweise mit dem Präfix `has_` oder `is_` oder so ähnlich benannt, damit man schon am Namen sieht, dass sie eine Bedingung prüfen. Allerdings ist das bei `ready()` eigentlich gar nicht nötig, weil die Methode gar keinen Rückgabewert braucht. Das Ende wird ja durch eine Ausnahme angezeigt. Diese syntaktische Form eine Ausnahme auszulösen, sollte man übrigens nicht mehr verwenden.
`get_copy()` ist ziemlich überflüssig. `N` ist ein schlechter Name und auch überflüssig. Eine Kopie von `self.Spiel` zu machen um ein neues `Sudoku`-Exemplar zu erstellen, scheint mir auch überflüssig -- warum nicht gleich eine Kopie von `self` machen!? Und dann ist diese ganze Methode nur noch ``return deepcopy(self)`` und es stellt sich die Frage, ob das echt eine eigene Methode sein muss.
`challange()` ist falsch geschrieben. Du solltest echt einen Spellchecker über Dein Programm laufen lassen.
`N` ist wieder, oder immer noch, ein schlechter Name.Die erste Zeile der Ausnahmebehandlung in `guess()` greift IMHO viel zu tief in die Datenstruktur ein. Der Sudoku-Löser sollte nicht den internen Aufbau von einzelnen Zellen kennen müssen. Das sollte man an die Zelle weiterdelegieren und die Zelle a.k.a. `Field` sollte auch die Ausnahme auslösen, wenn ihr die Möglichkeiten ausgegangen sind.
Was zum Henker ist `isod` für ein Name!?
Die Namensgebung der Klassen ist unglücklich. `SList` scheint eigentlich das `Sudoku` zu repräsentieren, während `Sudoku` von der Bedeutung eher `SudokuSolver` wäre. Man könnte auch die `SList` in `Field` umbenennen und das jetzige `Field` in `Cell` oder so ähnlich.
Importe sollte man nicht mitten im Modul verstecken sondern an den Anfang schreiben, damit man leicht sehen kann, wovon ein Modul abhängt.
Die `main()` ist wieder voller schlechter Namen. `slist` für etwas was gar keine Liste ist!? `a`, `b`, `s`? Solche Namen sind in der abgeschlossenen, begrenzten "Umgebung" einer "list comprehension" oder eines Generatorausdrucks in Ordnung, aber nicht als Namen mit einem grösseren "Lebensraum". Spezielle Rückgabewerte für Fehler sind schlechter Stil. Für so etwas wurden Ausnahmen erfunden. `get_todo()` sollte immer einen Wert liefern, oder halt eine Ausnahme auslösen, wenn das nicht geht.
Ich überlege gerade, wie man bei `get_todo()` ein `readout` hinbekommt, dass nicht die Länge 9 hat und dieser Test tatsächlich erreicht wird. Die Rückgabewerte sind abenteuerlich. Der Aufrufer hat das Dateiobjekt, kann sich also auch selber den Namen davon holen.
Mehr Leerzeichen wären schön. Der Style-Guide empfiehlt welche um Operatoren und Gleichheitszeichen (ausgenommen: Defaultargumente in Funktionsdefinitionen) und nach Kommas. Und bitte keine "Blöcke" in eine Zeile quetschen. Es ist einfach leichter lesbar, wenn da ein Zeilenumbruch steht, und entsprechend eingerückt ist.
http://paste.pocoo.org/show/141007/
Das ist mal die volle Dröhnung. Die Fehlerliste ist ja länger als der Quelltext
Aber herzlichen Dank fürs intensive durchlesen und die ehrliche Kritik.
Und: Ja, meine Englischkenntnisse sind doch äußerst begrenzt. in der 11. und 12. hab ich immer Englisch geschwänzt um meine Freundin zu besuchen. Andererseits hat das vielleicht einen Amoklauf verhindert... *nicht so ernst nehmen bitte*
Zum Thema:
Bloß ich hab immer noch keine Ahnung wie man ein "Array" in pythonischer Art ersetzt. Und wie ich die Daten besser übergeben kann.. ? Integer geht ja nicht wegen der führenden Nullen, die dann verschwinden.
Woher saugt ihr denn bloß immer wieder neue Namen?
Property ist nun drin - OK so?
€: Stimmt, deutsche Variablen gabs auch, sind jetzt hoffentlich raus.
Mein Englisch ist der running gag meiner Proggis^^
der Aufruf ``[1:9,1:9]`` liefert mir das gesamte Spielfeld als Liste von Feldern, die ich der Reihe nach "update"
Rückgabewerte wurden geändert.
Ich frage mich eben wie du in bestimmt kurzer Zeit durch den Quelltext steigst. Ich würde Stunden brauchen um da durchzusteigen...
Vielen Dank nochmal!
Das ist mal die volle Dröhnung. Die Fehlerliste ist ja länger als der Quelltext
Aber herzlichen Dank fürs intensive durchlesen und die ehrliche Kritik.
Und: Ja, meine Englischkenntnisse sind doch äußerst begrenzt. in der 11. und 12. hab ich immer Englisch geschwänzt um meine Freundin zu besuchen. Andererseits hat das vielleicht einen Amoklauf verhindert... *nicht so ernst nehmen bitte*
Zum Thema:
erledigt.^^@Snoda: "Ready" heisst auf deutsch "fertig" im Sinne von "bereit". "Ready" ist man vor einer Aufgabe und nicht danach. "Fertig" im Sinne von "Aufgabe ist fertig" heisst "Finished".
Ich stelle das ganze wieder auf 0-8 um. Ich hab mit aller Macht versucht die Sache in ein 1-9-"Array" zu pressen. Aber dieses +-1 zehrt gewaltig an der Übersicht.Wird demnächst erledigt.`Slist` würde ich nicht von `list` ableiten. Das Objekt hat dadurch einen Haufen Methoden, die keinen Sinn machen, zum Beispiel `sort()`. Was soll der Name bedeuten? Wenn man das fragen muss, ist das oft ein Zeichen, dass er nicht so gut gewählt ist.
Veränderbare Objekte als Defaultwerte für Argumente sind keine gute Idee, weil Defaultwerte nur *einmal* bei der Definition der Funktion ausgewertet werden. Und das `Slist.__init__()` in `data` Zeichenketten mit Ziffern entgegennimmt, wo die Klasse doch eigentlich auf Zahlen operiert, ist etwas "asymmetrisch". Dazu dann noch eine Ausnahme die behauptet, dass nur 1-9 erlaubt sind, wo doch ganz offensichtlich auch 0 geht. Die beiden Schleifen sollten besser von 0 bis 8 laufen und statt beim Index anzupassen, sollte man das wenn schon besser beim Aufruf von `Field()` machen. Statt `xrange()` und der Indexerei auf `data` wäre hier `enumerate()` "pythonischer".
Man sollte sich bei Computern angewöhnen, dass die Zählung bei 0 anfängt und gegebenenfalls nur an der Schnittstelle zum Benutzer den Index für die Ein- und Ausgabe anpassen. In der Programmlogik selbst ist man sonst ständig mit +1 und -1 im Gange und handelt sich subtile Fehler ein, wenn man nicht aufpasst. Es macht das Programm auf jeden Fall etwas komplizierter.
Bloß ich hab immer noch keine Ahnung wie man ein "Array" in pythonischer Art ersetzt. Und wie ich die Daten besser übergeben kann.. ? Integer geht ja nicht wegen der führenden Nullen, die dann verschwinden.
erledigt. Obwohl ich das eigenlich selber wusste -.- Naja, Dummheit...Die Zeilenfortsetungszeichen bei `__repr__()` und `__str__()` in `Slist` sind überflüssig. "Logische" Zeilen enden erst, wenn alle geöffneten Klammern auch wieder geschlossen sind. An der Stelle erkennt Python also ohne weitere Hinweise, dass der Ausdruck noch nicht zuende sein kann.
erledigt. Stimmt, keine Ahnung was ich mir da gedacht habe.So etwas wie ``"%s" % str(obj)`` ist übrigens zuviel des Guten. ``"%s" % obj`` bedeutet schon, dass `obj` in eine Zeichenkette umgewandelt werden muss, um an der Stelle eingesetzt zu werden. Das muss man vorher nicht schon mit `str()` machen.
Das mit den "s" ist trotz wenig Englischunterricht ziemlich peinlich... ! Und bei dem "S" ist mir nichts mehr eingefallen. Zumal die Variablennamen ja möglichst Englisch sein solltenNamen die mit Grossbuchstaben beginnen, sind per Konvention für Klassen vorbehalten. Ausnahme sind Namen, bei denen alle Buchstaben gross geschrieben sind, die sind üblich für Konstanten. Und `S` ist ein zu kurzer und nichtssagender Name. "Possibilities" schreibt man mit zwei "s".
Woher saugt ihr denn bloß immer wieder neue Namen?Wo hab ich denn deutsche Variablen? Hab ich was übersehen?Bei `Field.value` würde sich ein `property()` anbieten. Und bitte entscheide Dich mal, ob Du englische oder deutsche Namen vergibst.
Property ist nun drin - OK so?
€: Stimmt, deutsche Variablen gabs auch, sind jetzt hoffentlich raus.
Anderungen vorgenommen. Die Unterscheidung stammt von der ersten Programmlogik, da war das anders gelöst...Die "magischen" Methoden mit den Unterstrichen sollte man nur direkt aufrufen, wenn es keine andere Möglichkeit gibt. Also ``str(self.value)`` statt ``self.value.__str__()``. Die Fallunterscheidung in `Field.__repr__()` ist eigentlich überflüssig. Wenn der erste Zweig greift, hätte der zweite doch genau das gleiche Ergebnis gebracht.
``__str__ = __repr__`` ist überflüssig. `object.__str__()` ruft schon von sich aus ``repr(self)`` auf.
Namen geändert. Mir kam das die ganze Zeit seltsam vor, aber auf column bin ich nicht gekommen."row" und "line" heisst beides "Zeile". Wenn man Zeile und Spalte in Englisch ausdrücken will, dann redet man von "row" und "column". Beim Namen `getquad()` würde man eher erwarten, dass ein "Viertel" zurückgegeben wird.
Mein Englisch ist der running gag meiner Proggis^^
jetzt besser? Das mit dem "rechnen" im set hab ich nicht gerafft - was soll ich da Rechnen? Das set filtert mir auf einfache Art und Weise doppelte Werte raus, daher Type "set"`Field._update_posibilities()` ist nicht besonders effizient und IMHO auch nicht so logisch aufgebaut, wie es sein könnte. `pos` und `a` sind schlechte Namen. Wenn man schon mit `set()`\s arbeitet, sollte man auch ausnutzen, dass man damit "rechnen" kann. Eine Methode, die einen Wert zurückgibt, sollte das a) in jedem Zweig explizit tun, und nicht einfach in das implizite `None` am Ende laufen dürfen, und sollte b) möglichst immer Objekte vom gleichen "Duck-Typ" liefern und nicht `True` und `None`, was hier wahrscheinlich vom Aufrufer als "Falsch" interpretiert wird.
Du kennzeichnest die Methode durch den führenden Unterstrich als "intern", rufst sie dann aber von einer anderen Klasse aus auf!?
changed ist nun ein Attribut geworden. In der ersten Version wurde changed von Field.update_possibilities() geändert - imho noch grausamer als der RückgabewertBei der doch leicht komplizierten Implementierung von `SList.__getitem__()` ist mir nicht so wirklich klar, was ein Aufruf mit ``[1:9,1:9]`` genau liefert. It Dir das klar? Ich meine bis zu welcher Ebene da kopiert wird, und ob das in `Sudoku.update()` überhaupt Sinn macht!? Warum ist `changed` ein Attribut und kein lokaler Name?
der Aufruf ``[1:9,1:9]`` liefert mir das gesamte Spielfeld als Liste von Feldern, die ich der Reihe nach "update"
Ich fand die Lösung auch ziemlich unsauber - aber irgendwie einfach. Sonst müsste ich das Ergebnis ja durch die gesamte Rekursion zurück schleifen, sprich jeder Lösungsversuch müsste eine Referenz von "self" zurückgeben... damit das fertige Sudoku am Anfang wieder raus kommt, oder? Also statt der Exeption mit True/False Rückgaben arbeiten?Methoden, die einen Wahrheitswert zurückgeben werden üblicherweise mit dem Präfix `has_` oder `is_` oder so ähnlich benannt, damit man schon am Namen sieht, dass sie eine Bedingung prüfen. Allerdings ist das bei `ready()` eigentlich gar nicht nötig, weil die Methode gar keinen Rückgabewert braucht. Das Ende wird ja durch eine Ausnahme angezeigt. Diese syntaktische Form eine Ausnahme auszulösen, sollte man übrigens nicht mehr verwenden.
korrigiert`get_copy()` ist ziemlich überflüssig. `N` ist ein schlechter Name und auch überflüssig. Eine Kopie von `self.Spiel` zu machen um ein neues `Sudoku`-Exemplar zu erstellen, scheint mir auch überflüssig -- warum nicht gleich eine Kopie von `self` machen!? Und dann ist diese ganze Methode nur noch ``return deepcopy(self)`` und es stellt sich die Frage, ob das echt eine eigene Methode sein muss.
`challange()` ist falsch geschrieben. Du solltest echt einen Spellchecker über Dein Programm laufen lassen. Smile `N` ist wieder, oder immer noch, ein schlechter Name.
Puh, das geht jetzt echt in die Tiefe. Ich bin aber durchaus der Meinung dass die Funktion dort an der rechten Stelle ist. Die Zelle kann an sich is ja eigentlich nix ausrichten, sondern nur über die Referenz aufs Feld zugreifen. Damit hab ich genau den gleichen Effekt... (?)Die erste Zeile der Ausnahmebehandlung in `guess()` greift IMHO viel zu tief in die Datenstruktur ein. Der Sudoku-Löser sollte nicht den internen Aufbau von einzelnen Zellen kennen müssen. Das sollte man an die Zelle weiterdelegieren und die Zelle a.k.a. `Field` sollte auch die Ausnahme auslösen, wenn ihr die Möglichkeiten ausgegangen sind.
Hab mir was besseres einfallen lassen xDWas zum Henker ist `isod` für ein Name!?
geändert, das war echt grottig ;(Die Namensgebung der Klassen ist unglücklich. `SList` scheint eigentlich das `Sudoku` zu repräsentieren, während `Sudoku` von der Bedeutung eher `SudokuSolver` wäre. Man könnte auch die `SList` in `Field` umbenennen und das jetzige `Field` in `Cell` oder so ähnlich.
Importe sollte man nicht mitten im Modul verstecken sondern an den Anfang schreiben, damit man leicht sehen kann, wovon ein Modul abhängt.
besser?Die `main()` ist wieder voller schlechter Namen. `slist` für etwas was gar keine Liste ist!? `a`, `b`, `s`? Solche Namen sind in der abgeschlossenen, begrenzten "Umgebung" einer "list comprehension" oder eines Generatorausdrucks in Ordnung, aber nicht als Namen mit einem grösseren "Lebensraum". Spezielle Rückgabewerte für Fehler sind schlechter Stil. Für so etwas wurden Ausnahmen erfunden. `get_todo()` sollte immer einen Wert liefern, oder halt eine Ausnahme auslösen, wenn das nicht geht.
Der Test konnte nicht erreicht werden, da bei zu kurzen Files immer ein "" zurückgegeben wird, was dann zu "000000000" umgewandelt wird. Also überflüssig...Ich überlege gerade, wie man bei `get_todo()` ein `readout` hinbekommt, dass nicht die Länge 9 hat und dieser Test tatsächlich erreicht wird. Die Rückgabewerte sind abenteuerlich. Der Aufrufer hat das Dateiobjekt, kann sich also auch selber den Namen davon holen.
Rückgabewerte wurden geändert.
Hab versucht alles zu erwischen...Mehr Leerzeichen wären schön. Der Style-Guide empfiehlt welche um Operatoren und Gleichheitszeichen (ausgenommen: Defaultargumente in Funktionsdefinitionen) und nach Kommas. Und bitte keine "Blöcke" in eine Zeile quetschen. Es ist einfach leichter lesbar, wenn da ein Zeilenumbruch steht, und entsprechend eingerückt
Ich frage mich eben wie du in bestimmt kurzer Zeit durch den Quelltext steigst. Ich würde Stunden brauchen um da durchzusteigen...
Vielen Dank nochmal!
Hello world!
Vor einiger Zeit habe ich zum Thema Sudoku die folgende Seite von Peter Norvig gefunden: Solving Every Sudoku Puzzle.
Viel Spaß!
Viel Spaß!
-
BlackJack
@Snoda: Ein "Array" setzt man in Python am besten mit einer Liste um. Es sei denn man hat besondere Anforderungen wie Rechnen mit Matrizen, dann verwendet man `numpy`, oder man will viele Zahlen im Wertebereich der üblichen, hardwarenahen Zahlentypen aus anderen Programmiersprachen speichern, dann bietet sich das `array`-Modul aus der Standardbibliothek an.
Warum kannst Du die Daten nicht als "2D"-Liste mit Zahlen übergeben? Mit `None` oder 0 für leere Felder.
Gute, passende Namen zu finden ist unabhängig von der Programmiersprache eine der Herausforderungen beim Programmieren. An der Stelle bei dem `S` müsste es aber kein neuer Name sein, wenn der auf der rechten Seite des ``=`` passt, kann man den für das Attribut ja auch verwenden.
Beim "rechnen" mit `set()`\s meinte ich die Mengenoperationen, die der Datentyp kennt. Also das man von der Menge der Möglichkeiten die Menge der bereits verwendeten Zahlen abzieht:
`chain()` kommt aus dem `itertools`-Modul. Zumindest solltest Du in der Schleife aber das `remove()` durch `discard()` ersetzen, dann gibt es nämlich keinen `KeyError` mehr, um den man sich kümmern müsste.
Bei der `SudokuSolver.update()` wäre IMHO eine ``while True``-Schleife idiomatischer, die im Schleifenkörper am Ende überprüft, ob sich nichts geändert hat, und dann ``break`` verwendet, um die Schleife zu verlassen.
`has_finished()` hat eigentlich keinen Rückgabewert. Es wird *immer* `False` zurückgegeben, damit ist der Rückgabewert wertlos, den weiss man ja schon vorher. Da würde ein einfaches ``return`` ohne Wert genügen. Man kommt aber auch ganz ohne ``return`` aus, indem man eine Abfrage formuliert, ob alle Felder einen Wert gesetzt haben und dann die Ausnahme auslöst:
Die Zelle weiss selber besser über ihre "Innereien" Bescheid, und der Solver sollte es nicht so genau wissen müssen. Die Zelle kann selber erkennen, wenn ihr die Möglichkeiten ausgegangen sind, und das mit der Ausnahme quittieren. Die Ausnahmebehandlung in `SudokuSolver.guess()` sähe dann so aus:
Und das `Field` bekäme eine zusätzliche Methode:
Warum kannst Du die Daten nicht als "2D"-Liste mit Zahlen übergeben? Mit `None` oder 0 für leere Felder.
Gute, passende Namen zu finden ist unabhängig von der Programmiersprache eine der Herausforderungen beim Programmieren. An der Stelle bei dem `S` müsste es aber kein neuer Name sein, wenn der auf der rechten Seite des ``=`` passt, kann man den für das Attribut ja auch verwenden.
Beim "rechnen" mit `set()`\s meinte ich die Mengenoperationen, die der Datentyp kennt. Also das man von der Menge der Möglichkeiten die Menge der bereits verwendeten Zahlen abzieht:
Code: Alles auswählen
def update_possibilities(self):
values = (field.value
for field in chain(self.row(), self.column(), self.frame())
if field is not self)
self.possibilities.difference_update(values)
# ...Bei der `SudokuSolver.update()` wäre IMHO eine ``while True``-Schleife idiomatischer, die im Schleifenkörper am Ende überprüft, ob sich nichts geändert hat, und dann ``break`` verwendet, um die Schleife zu verlassen.
`has_finished()` hat eigentlich keinen Rückgabewert. Es wird *immer* `False` zurückgegeben, damit ist der Rückgabewert wertlos, den weiss man ja schon vorher. Da würde ein einfaches ``return`` ohne Wert genügen. Man kommt aber auch ganz ohne ``return`` aus, indem man eine Abfrage formuliert, ob alle Felder einen Wert gesetzt haben und dann die Ausnahme auslöst:
Code: Alles auswählen
def has_finished(self):
if all(field.value for field in self[1:9, 1:9]):
raise Finished(self)Code: Alles auswählen
except SudokuError:
self[field.x, field.y].remove_possibility(field.value)
returnCode: Alles auswählen
def remove_possibility(self, value):
self.possibilities.remove(value)
if not self.possibilities:
raise SudokuError("keine Moeglichkeiten vorhanden")