Kritik am code

Fragen zu Tkinter.
Antworten
misterjosu
User
Beiträge: 44
Registriert: Samstag 29. Dezember 2012, 21:40

Hey,

Da mir geraten wurde meine Programme in klassen zu packen hab ich das mal am Beispiel von Tic Tac Toe versucht Kritik am Code ist erwünscht und ich hoffe dass es hier besser ist mit der OOP.

Was ich noch anders machen sollte ist das tauschen der Grafiken , aber ich weiß nicht wie ich das Widget konfigurieren soll wenn ich dem Widget keinen Namen zugeteilt habe.

Was auch noch anders gemacht werden sollte ist dass Toplevel Window, dass ich benutze um anzuzeigen welcher Spieler im Moment an der Reihe ist. Wenn ich das ganze in der bind anweisung aber an ein Frame binde funktioniert das ganze nicht.

Danke schon mal im Voraus für Kritik.

Code: Alles auswählen

from tkinter import *


class Main(object):
    
    meta =  "-- Version 0.1 --\n Written by Josu\n Tic Tac Toe in Python 3.2 " 
    
    player = 1 # 1/2
    liste = [[" "," "," "],[" "," "," "],[" "," "," "]]
    root = Tk()
    options = Toplevel(root)

    icon_c = PhotoImage(file="icon_clean.gif")
    icon_x = PhotoImage(file="icon_x.gif")      # player 1
    icon_o = PhotoImage(file="icon_o.gif")      # player 2

    player_1 = PhotoImage(file="player_1.gif")
    player_2 = PhotoImage(file="player_2.gif")
    
    def __init__(self):
        pass

    def new_game(self):
        self.liste = [[" "," "," "],[" "," "," "],[" "," "," "]]
        self.create_main_window()

    def toggle(self):
        if self.player == 1:
            self.player = 2
            Label(self.options, image= self.player_2, width = 124, height = 124).grid(row= 0, rowspan = 2, column=0)
            Button(self.options, text="New Game", width= 30, bg = "red", command=self.new_game).grid(row=1, column=1)
            print("setted to 2")
        elif self.player == 2:
            self.player = 1
            Label(self.options, image= self.player_1, width = 124, height = 124).grid(row= 0, rowspan = 2, column=0)
            Button(self.options, text="New Game", width= 30, bg = "blue", command=self.new_game).grid(row=1, column=1)
            print("setted to 1")

    def pressed(self, event):
        
        grid_info = event.widget.grid_info()
        x = grid_info["row"]
        y = grid_info["column"]
        print(x)
        print(y)

        if self.player == 1:
            if self.liste[int(x)][int(y)] == " ":
                print("s")
                self.liste[int(x)][int(y)] = "X"
                Label(self.root, image=self.icon_x, width=128, height=128).grid(row=x, column=y)
                self.toggle()

        elif self.player == 2:
            if self.liste[int(x)][int(y)] == " ":
                print("s")
                self.liste[int(x)][int(y)] = "O"
                Label(self.root, image=self.icon_o, width=128, height=128).grid(row=x, column=y)
                self.toggle()
                
        mainloop()
        
    def create_main_window(self):
        """ Erstellt das Hauptfenster UND das options fenster """
        self.root.title("Spielfeld")

        for x in range(0, 3):
            for y in range(0, 3):        
                Label(self.root, image=self.icon_c, width=128, height=128).grid(row=x, column=y)

        self.root.bind("<Button-1>", self.pressed)

        self.options.title("options")

        Label(self.options, image= self.player_1, width = 124, height = 124).grid(row= 0, rowspan = 2, column=0)
        Label(self.options, text = self.meta).grid(row=0, column=1)
        Button(self.options, text="New Game", width= 30, bg = "green", command=self.new_game).grid(row=1, column=1)
        
        mainloop()

        
Main().create_main_window()

BlackJack

@misterjosu: Alles was zum Zustand des `Main`-Objekts gehört hat auf Klassenebene nichts zu suchen, sondern gehört in der `__init__()` an das Exemplar von `Main` gebunden und nicht an die Klasse. Die Namen die dann noch auf Klassenebene verbleiben, würde ich gemäss der Namenskonvention für Konstanten dann komplett in Grossbuchstaben schreiben.

Der Name `Main` ist zu generisch, davon weis niemand wofür dieses Objekt eingentlich steht. So etwas wie `TicTacToe` wäre zum Beispiel passender.

Dieses ständige erstellen von Widgets ist ein wirklich richtig böser Programmfehler. So kann man mit Ressourcen einfach nicht umgehen. Ein Objekt muss man nicht (direkt) an einen Namen binden, um darauf später zugreifen zu können, man kann es auch in eine Datenstruktur stecken. In eine Liste zum Beispiel. Die Labels, die die Zellen des Spielfelds darstellen, kann man genau so in eine verschachtelte Liste stecken um dann mit den gleichen Koordinatenwerten auch auf das Label zur Anzeige zuzugreifen und das zu ändern.

Quelltext sollte sich nicht wiederholen. Identischer sowieso nicht, aber auch sehr ähnlicher nicht. Das verletzt das DRY-Prinzip (Don't Repeat Yourself) und macht Code fehleranfällig und schlecht wart- und erweiterbar. Weil man immer alle gleichen oder ähnlichen Stellen in gleicher Weise verändern muss, und dabei früher oder später etwas übersieht, oder geringfügig anders verändert. Das betrifft die literale Liste für das leere Spielfeld, die zweimal vorhanden ist, die beiden Zweige in der `toggle()`-Methode, ebenso die in der `pressed()`-Methode.

Bei ``if``/``elif`` schreibe ich in der Regel einen ``else``-Zweig. Wenn der einen Fall betrifft, der eigentlich nie eintreffen kann, dann dokumentiere ich dort warum das nie passieren kann und löse eine Ausnahme aus. Meistens mit einem ``assert False, 'Grund...'``. Denn ab und zu passiert so etwas was niemals passieren kann, dummerweise doch, und dann möchte man das als Programmierer normalerweise wissen und das Programm nicht einfach weiterlaufen lassen als wäre nichts gewesen. :-)

Die zwei Fenster sind in der Tat komisch. Das kann man auch in mehreren Frames in einem Fenster unterbringen. Irgendetwas machst Du falsch, aber „funktioniert das ganze nicht” ist halt keine ausreichende Beschreibung um da irgendwie weiterhelfen zu können.

Der `mainloop()`-Aufruf sollte nur einmal im Programm passieren. Diese Funktion kehrt erst zurück, wenn das Hauptfenster zerstört wurde. Du rufst sie bei jedem Mausklick auf eine Zelle im TicTacToe-Gitter aus, und jedes mal wenn der Spieler auf die Schaltfläche für ein neues Spiel drückt. Das wird nur eine begrenzte Anzahl von klicks gut gehen bevor das Programm abstürzt. Das ist ebenfalls ein ziemlich böser Programmfehler. Insbesondere in der `pressed()`-Methode verstehe ich es auch überhaupt nicht. Diese Methode wird von der Tk-Hauptschleife aus aufgerufen und wenn die Methode abgearbeitet ist, dann kehrt der Programmfluss zu dieser Hauptschleife zurück. Es macht gar keinen Sinn hier eine *Neue* zu starten.

Ich glaube ich habe das schon mal gesagt, dass man die Koordinaten nicht aus dem GUI-Layout auslesen sollte. Damit missbraucht man die GUI als Datenspeicher und schränkt sich das Layout damit unnötig ein, beziehungsweise macht man sich vom GUI-Layout abhängig, weil man das nicht mehr frei ändern kann ohne ein Stück Programmlogik damit zu beeinflussen. Die Koordinaten könnte man an die Rückruffunktionen für die einzelnen Labels mit `functools.partial()` binden.

Bei `create_main_window()` ist sowohl der Name als auch der DocString falsch, denn beide Fenster werden auf Klassenebene erzeugt und nicht in dieser Methode. Das ist wie gesagt der falsche Ort dafür. Aber auch diese Methode ist nicht wirklich ein guter Ort, zumindest nicht wenn die von irgendwo anders als von der `__init__()` aus aufgerufen werden soll. Denn die `__init__()` sollte ein Objekt in einen konsistenten, benutzbaren, initialisierten Zustand versetzen.

Die Schleife, die die `Label` erzeugt, sollte sich an der Datenstruktur vom Spielfeld orientieren und keine fixen Annahmen über die Grösse des Feldes machen.

Der OOP-Entwurf könnte besser sein. Spiellogik und GUI gehören getrennt. Mindestens das TicTacToe-Gitter ist Kandidat für eine eigene Klasse die von `Frame` abgeleitet werden kann.
Antworten