ConnectFour

Stellt hier eure Projekte vor.
Internetseiten, Skripte, und alles andere bzgl. Python.
Antworten
Karl
User
Beiträge: 252
Registriert: Freitag 29. Juni 2007, 17:49

Montag 9. Juli 2007, 13:02

Hi.
Ich hatte ja letztens vor, ein konsolenbasiertes Schachspiel zu programmieren aber angesichts der Tatsache, dass ich neu in Python bin und noch nicht viel mit OOP gemacht hab, hab ich mir gedacht, dass Schach fürs erste doch etwas zu Komplex für das erste Python-Projekt wäre.
Also hab ich mich für 4-Gewinnt entschieden, da das sehr viel simpler ist.

Was haltet ihr von meinem Code?

Code: Alles auswählen

# -*- coding: cp1252 -*-
class Board:
    def __init__(self, rows, colums):
        self.board = [0] * rows
        self.rows = rows
        self.colums = colums
        for x in range(0, rows):
            self.board[x] = [0] * colums

    def __str__(self):
        board = ""
        for i in range(0, self.colums):
            board = board +  " " + str(i + 1) + " " * 3
        board = board + "\n\n"
        for row in range(0, self.rows):
            for col in range(0, self.colums):
                if self.board[row][col] == 0:
                    board = board + " - | "
                else:
                    board = board + " " + str(self.board[row][col]) + " | "
            board = board + "\n" + (self.colums * 5 - 1) * "-" + "\n"
        return board

    def __getitem__(self, key):
        return self.board[key]
    
    def ChangeValue(self, row, col, value):
        self.board[row][col] = value

class Player:
    def __init__(self, id, name):
        self.id = id
        self.name = name

class ConnectFour:
    def __init__(self, player1, player2, rows = 6, colums = 7):
        self.board = Board(rows, colums)
        self.player = [Player("X", player1), Player("O", player2)]
        self.turn = 0

    def __str__(self):
        return '-' * 30 + "\n" + str(self.board) + "\nPlayer 1: " + self.player[0].name + " [" + self.player[0].id + "]\nPlayer 2: " + self.player[1].name + " [" + self.player[1].id + "]" + "\n" + '-' * 30 + "\n"

    def ChangeTurn(self):
        if self.turn == 0:
            self.turn = 1
        else:
            self.turn = 0
    
    def DropDisc(self, col):
        result = ""
        for i in range(0, self.board.rows):
            if self.board[i][col] == 0:
                result = i
            else:
                break
        if result == "":
            raise -1
        self.board[result][col] = self.player[self.turn].id

    def CheckWin(self):
        # Check vertical
        count = 0
        for row in range(0, self.board.rows):
            for col in range(0, self.board.colums):
                if self.board[row][col] == self.player[self.turn].id:
                    count += 1
                    if count == 4:
                        return self.player[self.turn].id
                else:
                    count = 0
        
        # Check horizontal
        count = 0
        for col in range(0, self.board.colums):
            for row in range(0, self.board.rows):
                if self.board[row][col] == self.player[self.turn].id:
                    count += 1
                    if count == 4:
                        return self.player[self.turn].id
                else:
                    count = 0
        
        # Check diagonal
        count1 = 0
        count2 = 0
        for row in range(0, self.board.rows - 3):
            for col in range(0, self.board.colums - 3):
                for i in range(0, 4):
                    if self.board[row + i][col + i] == self.player[self.turn].id:
                        count1 += 1
                    else:
                        count1 = 0
                    if self.board[row + i][col + 3 - i] == self.player[self.turn].id:
                        count2 += 1
                    else:
                        count2 = 0
                if count1 == 4 or count2 == 4:
                    return self.player[self.turn].id
        return 0

    def play(self):
        while(1):
            try:
                print str(self.player[self.turn].name) + " ist an der Reihe\nWo möchtest du setzen?"
                row = input()
                self.DropDisc(row - 1)
                print str(self) + "\n"
                if self.CheckWin() != 0:
                    print self.player[self.turn].name + " Hat gewonnen!!!"
                    break
                self.ChangeTurn()
            except:
                print "\nUngültige Eingabe!\n"

x = ConnectFour("Karl", "Horst")
x.play()
Ich hab jetzt noch vor eine Netzwerkfunktion zu implementieren und evtl eine richtiges GUI.
Meint ihr der Code lässt sich dafür weiterverwenden?
Ich muss mir erstmal die ganzen Module anschauen, die ich überhaupt für die Netzwerkfunktion brauche und mir das dann aneignen, deswegen kann ich das noch relativ schlecht beurteilen.
Leonidas
Administrator
Beiträge: 16024
Registriert: Freitag 20. Juni 2003, 16:30
Kontaktdaten:

Montag 9. Juli 2007, 15:45

Hallo Karl,

nimm es mir nicht übel, aber ich habe in den letzten Jahren herausgefunden, dass Kritik wohl der beste Weg zu Erkenntniss ist und dass es mir auch viel mehr Spaß macht, Programme zu kritisieren als zu loben :P Daher bitte ich dich, dies als Konstruktive Kritik anzusehen. Zuerst vielleicht mal aus formeller Sicht: du solltest den PEP8 lesen und auf dein Programm anwenden, weil es doch einige Dinge gibt, die ich anders machen würde: insbesondere Stecken da die CamelCase Funktionasnamen hervor. Eine Shebang-Zeile wäre auch nicht vergekt, ebenso wie den Inport-Trick (``if __name__ == '__main__':``). Ansonsten würde ich es gerne sehen, wenn dein Programm mehr Kommentare enthalten würde. Ich weiß, das Kommentare schreiben langweilig ist, aber ich nutze sie immer häufiger, weil ich lieber sie lese als zu entziffern versuche was ein Stück Code macht und wozu es überhaupt da ist.
Eine andere Sache ist bei dir das ``range``: statt ``range(0, rows)`` würde ich eher ``range(rows + 1)`` machen, weil ``range`` eigentlich immer von Null anfängt. ``__str__`` würde ich wohl auch etwas anders läsen. In den ``for``-Schleifen sind einbuchstabige Bezeichner etwas suboptimal, wenn dann höchstens ``i`` für Integer-Werte nutzen, aber auch da könnte man überlegen, sich einen aussagekräftigeren Namen auszudenken. Eine Variable ``id`` zu benennen ist jetzt nicht sonderlich schlimm, aber ``id()`` ist ein Builtin und gibt, nun ja, die ID eines Objektes zurück. Bei ``ConnectFour.__init__`` würde ich die Player wohl als Tupel entgegennehmen, statt zweier einzelner Parameter. ``while(1):`` in Zeile 103 kann man besser als ``while True:`` schreiben. Ich würde noch überlegen, ob wirklich eine Mini-Klasse ``Player`` nötig ist. Zudem nutzt du sehr viele Indizes mit Magischen Nummern. Zum Beispiel in ``ChangeTurn()``: dort sind drei Zahlen und keiner dieser Zahlen sagt dem Leser irgendwas, was sie zu bedeuten haben. Also entweder Kommentieren und in Konstanten auslagern oder irgendwie anders implementierenn, dass nicht so viele Zahlen drin stehen. Von der unfassbar langen Zeile 42 mal ganz zu schweigen.
My god, it's full of CARs! | Leonidasvoice vs Modvoice
BlackJack

Montag 9. Juli 2007, 16:12

So ein doofes Programm! Ich werde gefragt wo ich setzen möchte, ohne erklärt zu bekommen, was ich denn eigentlich eingeben muss. Gebe ich etwas falsches ein, bekomme ich einfach nur "Ungültige Eingabe". Abbrechen lässt sich das Programm nicht -- auch bei Strg+C bekomme ich nur wieder "Ungültige Eingabe". Das ist insbesondere dann blöd, wenn das Programm einen Fehler enthält, dann kommt an der Stelle nämlich immer nur diese nichtssagende Meldung und man kann es nicht abbrechen.

Das Spielfeld sollte man grundsätzlich vor dem Zug angezeigt bekommen. Auch vor dem allerersten. Dann kann man zumindest ahnen was man eingeben soll.

Es sind ein paar Zeilen länger als 80 Zeichen, insbesondere eine Monsterzeile. Und die Methodennamen halten sich nicht an den Python Style Guide.

Der Board-Aufbau in der `Board.__init__()` ist komplizieter als er sein müsste und auch etwas ungewöhnlich angeordnet. Die "äussere" Liste würde ich mit `None`\s initialisieren statt mit 0en. Oder aber gleich das gesamte Spielfeld in einer "list comprehension" aufbauen.

Code: Alles auswählen

    def __init__(self, rows, columns):
        self.rows = rows
        self.columns = columns
        self.board = [['-'] * columns for dummy in xrange(rows)]
Als Inhalt habe ich gleich das Zeichen genommen, was auch für leere Felder angezeigt werden soll. Damit wird die Anzeige einfacher. Die sehr umständlich formuliert ist. Mit dem Formatierungsoperator und der `join()`-Methode bekommt man das kompakter hin. Selbst bei einer "langen" Version mit Schleifen sollte man über die Elemente direkt iterieren und die Indexe weglassen.

Code: Alles auswählen

    def __str__(self): 
        header = ''.join(' %d  ' % (i + 1) for i in xrange(self.columns))
        board = ('|'.join(' %s ' % cell for cell in row) for row in self.board)
        separator = '\n%s\n' % ('-' * (self.columns * 4 - 1))
        return '%s\n\n%s%s' % (header, separator.join(board), separator)
Beim `Player` wird die eingebaute Funktion `id()` von einem lokalen Namen überschattet.

Bei `ConnectFour.DropDisc()` wird `result` erst an eine Zeichenkette gebunden, später aber an eine Zahl und als solche dann auch verwendet. Das ist irreführend. Auch hier wäre `None` besser.

``raise -1`` führt nur indirekt zu einer Ausnahme, nämlich zu einem `TypeError`, weil -1 keine gültige Ausnahme ist.

Die `CheckWin`-Methode ist mir persönlich ein wenig zu lang, ich hätte da drei draus gemacht. Und auch hier würde ich `None` statt 0 zurückgeben, falls keiner gewonnen hat.

Endlosschleifen schreibt man üblicherweise als ``while True:``. Und die `input()`-Funktion sollte man nicht benutzen. Dort kann der Benutzer fast beliebigen Python-Code eingeben. In dieser Methode ist auch der Grund warum alle Fehler im Programm "verschluckt" werden: ``except:`` ohne konkrete Ausnahmen anzugeben ist kein guter Stil.

Das Hauptprogramm sollte auch in eine Funktion, damit man das Modul auch importieren kann ohne dass das Spiel losgeht. Zum Beispiel für Testcode oder zum Ausprobieren von einzelnen Funktionen in einer Python-Shell.
Karl
User
Beiträge: 252
Registriert: Freitag 29. Juni 2007, 17:49

Montag 9. Juli 2007, 16:18

Erstmal: Ich nehms dir überhaupt nicht übel, ich danke dir, dass du dir die Zeit genommen hast um mir diese paar Kritikpunkte zu schreiben ;)

Das mit dem if __name__ == '__main__': hab ich schon öfter irgendwo gesehn aber ich hab nie ganz verstanden, was das sein soll. Aber ich glaub bei mir hat's grad klick gemacht ;)
Bei den Kommentaren hast du eigentlich recht, es ist sehr schwer ein fremdes Programm zu lesen, weil derjenige oft andere Denkansätze hat oder etwas völlig anders macht, da würden Kommentare echt helfen. Ich werd versuchen das in Zukunft zu ändern.
Manchmal sind meine Variablennamen echt schlecht gewählt.
Naja, beim schreiben des Codes denkt man eben nicht daran, dass man irgendwann den Code selbst nicht mehr verstehen wird, wenn man nichtssagende Variablennamen benutzt.
Die Klasse für die Spieler ist wahrscheinlich echt unnötig und macht das ganze noch komplizierter, aber irgendwie kams mir, als ich die Idee hatte richtig vor.
Manche Sachen sind wohl auch echt unpraktisch gelöst, also nicht auf flexibilität ausgelegt.
Aber eine Frage hätte ich noch, warum ist while True: besser?
BlackJack

Montag 9. Juli 2007, 16:44

Weil's deutlicher ist. Die Schleife wird solange ausgeführt wie die Bedingung "wahr" ist, und da ist "Wahr", also `True` der Wert, der das am direktesten ausdrückt. Wenn da eine 1 steht muss man sich zusätzlich klarmachen, dass ``bool(1) == True`` gilt. Und warum gerade 1? 42 würde es auch tun. Oder 'foo', oder (1, 2, 3) oder…
Redprince
User
Beiträge: 128
Registriert: Freitag 22. Oktober 2004, 09:22
Wohnort: Salzgitter
Kontaktdaten:

Montag 9. Juli 2007, 16:45

Vor einiger Zeit wurde etwas ähnliches hier im Forum begonnen, vielleicht hilft dir das ja weiter.
Vielleicht magst du dir ja auch mal meine Umsetzung des Spielfeldes ansehen, welche ich für eines meiner Projekte gebastelt habe. Die Prüfroutine geht vom letzten gesetzten Stein aus und sollte somit performanter sein als deine Methode, welche anscheinend das gesamte Spielfeld durchläuft!
I am not part of the allesburner. I am the [url=http://allesburner.de]allesburner[/url].
Leonidas
Administrator
Beiträge: 16024
Registriert: Freitag 20. Juni 2003, 16:30
Kontaktdaten:

Montag 9. Juli 2007, 16:46

Karl hat geschrieben:Aber eine Frage hätte ich noch, warum ist while True: besser?
Weil 1. die Klammern unnötig sind und sie nur der Lesbarkeit willen hinzugefügt werden können und in diesem Fall die Lesbarkeit ohne Klammern besser ist und 2. weil ``True`` aussagekräftiger ist. ``while`` prüft ja eine Bedingung also prüft auf Wahr und Falsch. Ich finde es wesentlich klarer wenn man ``solange Wahl:`` im Code stehen hat und nicht ``solange 1:``. Technisch gesehen macht es auch keinen Unterschied, da ``True`` sich wie ``1`` und ``False`` sich wie ``0`` verhält, aber es ist eben vom Verständniss besser.
My god, it's full of CARs! | Leonidasvoice vs Modvoice
Karl
User
Beiträge: 252
Registriert: Freitag 29. Juni 2007, 17:49

Montag 9. Juli 2007, 17:00

Ok, jetzt zu BlackJack ;)
1. Dass das Spielbrett am Anfang nicht angezeigt wird stimmt, da fehlt noch ein print self vor der While-Schleife (bei mir hab ich das schon länger geändert aber das war dann wohl nachdem ich den Code hier gepastet hab)
2. Wie kann ich das Programm denn dazu bringen, dass es bei strg c abbricht?
3. Ein paar Zeilen sind echt zu lang, das werd ich dann wohl auch noch ändern müssen ;)
4. Zu dem __str__(): Ich programmiere wie gesagt ja noch nicht lange mit Python, so gut kenn ich mich nicht mit den Zig Möglichkeiten, die mir zur Auswahl stehen, wenn ich etwas ausgeben will, nicht aus. Ich werd mir das bei gelegenheit mal näher anschauen.
5. Ok, das Board zu initialisieren geht echt einfacher, danke :)
6. Die Sache mit dem id() hab ich gesehen, als "id" gehighlighted wurde, nur hab ich nicht gedacht, dass das jetzt irgenetwas bewirken würde. Aber ihr habt natürlich recht, lieber gleich ganz andere Namen wählen, damit keine Konflikte mit irgendwelchen reservierten Namen auftreten können.
7. Hm mit raise wusste ich nicht, dann änder ichs eben einfach zu 1 oder so, oder?
8. Ich hab mir auch überlegt die CheckWin-Methode in 3 Methoden aufzugliedern, nur hab ich mir gedacht, dass sie so lang ja auch nicht ist und das sowieso alles zusammengehört, da kann ichs auch grad in einer lassen. Ist wohl eher Geschmackssache.
9. Ok, kann sein, dass input() schlecht ist, soll ich dann raw_input nehmen?
Ich hatte mir halt gedacht, dass input() einfacher ist und ich eh einen Fehler ausgebe, wenn etwas nicht klappt. Hab ich's mir vielleicht zu einfach gemacht? :p
10. Das mit der main-Fuktion werd ich auch noch machen

Ich muss eh noch ein paar Sachen verändern, mir ist z.B grad aufgefallen, dass ich gar kein Unentschieden berücksichtig hab, dh die Überprüfung ob der Rückgabewert != 0 ist bringts auch nicht mehr usw.

Gibt es echt so einen Python Style Guide oder soetwas, oder ist das nur inofiziell? Link? :D

Edit: Irgendwie bin ich viel zu langsam :D
Die Umsetzung, wie du das gemacht hast, schau ich mir auch mal an, es klingt irgendwie auch logisch, nur vom letztem Spielstein auszugehen ;)

Ob True oder 1 jetzt besser ist, ist dann wohl auch Geschmackssache, ich bin es eh gewohnt, 1 und True gleichzusetzen (auch wenn das bei Pythoniern (sagt man das so? :p) vllt nicht so ist.
Aber dann schreib ich eben True, wenn gleich 2 Kommentare dazu abgegeben wurden, vertrau ich mal darauf :D
BlackJack

Montag 9. Juli 2007, 18:17

2. Einfach nicht *jede* Ausnahme verschlucken, dann geht das automatisch. (Unter Windows ist es Strg+Z).

7. Nein 1 ist auch falsch. Es sollte schon eine Ausnahme sein, entweder eine Klasse oder eine Instanz.

8. Ist Geschmackssache, aber einzeln liessen sich die drei Methoden zum Beispiel auch separat testen.

9. Jup, `raw_input()` und explizit in eine Zahl umwandeln ist besser.

Und noch ein Link: PEP 0008 - Style Guide
Leonidas
Administrator
Beiträge: 16024
Registriert: Freitag 20. Juni 2003, 16:30
Kontaktdaten:

Montag 9. Juli 2007, 18:46

Karl hat geschrieben:Ob True oder 1 jetzt besser ist, ist dann wohl auch Geschmackssache, ich bin es eh gewohnt, 1 und True gleichzusetzen (auch wenn das bei Pythoniern (sagt man das so? :p) vllt nicht so ist.
Es ist bei normalen Menschen (also nichtprogrammieren) nicht so, dass eins und Wahr gleichgesetzt wird. In C ist das der Fall, und viele Programmiersprachen haben das übernommen, aber das heißt nicht, dass man jetzt in Python 1 für Wahr einsetzen muss um sich als cooler Programmierer zu outen :wink:
My god, it's full of CARs! | Leonidasvoice vs Modvoice
Antworten