4
\$\begingroup\$

How well is this Tic Tac Toe code written? Where may I improve it? This is meant to be a platform for a machine learning algorithm.

The code is written so any player may play first. Once a player plays first and starts the game, the code enforces turns. Overwrite protection is also enforced.

#! /usr/bin/python3
class xo:
    def __init__(self):
        self.board=[[0,0,0],[0,0,0],[0,0,0]];
        self.sym=[' ','0','X'];
        self.turn=0;
        self.modResX=-1;
        self.modResO=-1;
        self.won=False;

    def setPos(self,posx,posy,who):
        if (who>=0 & who<3):
            self.board[posx][posy]=who;
        return 0;

    def setX(self,posx,posy):
        # check if X is playing first.
        if self.turn==0:
            self.modResX=0;
            self.modResO=1;
        # check if X is not playing out of turn.
        if self.turn%2==self.modResX:
            # check if we are overwriting a position
            if (self.board[posx][posy]==0):
                self.board[posx][posy]=2;
                self.turn+=1;
                self.win(2);
                return 0;
            else:
                return -2;
        else:
            return -1;

    def setO(self,posx,posy):
        # check if O is playing first.
        if self.turn==0:
            self.modResX=1;
            self.modResO=0;
        # check if O is not playing out of turn.
        if self.turn%2==self.modResO:
            # check if we are overwriting a position
            if (self.board[posx][posy]==0):
                self.board[posx][posy]=1;
                self.turn+=1;
                self.win(1);
                return 0;
            else:
                return -2;
        else:
            return -1;

    def win(self,who):
        win=False;
        if self.board[0]==[who, who, who]:
            win=True;
        if self.board[1]==[who, who, who]:
            win=True;
        if self.board[2]==[who, who, who]:
            win=True;
        if ([self.board[0][0], self.board[1][1], self.board[2][2]]==[who,who,who]):
            win=True;

        transList=list(map(list, zip(*self.board)))
        if transList[0]==[who, who, who]:
            win=True;
        if transList[1]==[who, who, who]:
            win=True;
        if transList[2]==[who, who, who]:
            win=True;
        if ([transList[0][0], transList[1][1], transList[2][2]]==[who,who,who]):
            win=True;
        self.won=win;
        return win;

    def showBoard(self):
        for i in range(0, 3):
            for j in range (0,3):
                if j<2:
                    print (self.sym[self.board[i][j]],'| ',end="",flush=True)
                else:
                    print (self.sym[self.board[i][j]],end="",flush=True)
            print(end="\n")
        print(end="\n")
        return 0;

def main():
    print("Hello");
    g=xo();
    g.showBoard();
    print(g.setX(2,2));
    g.showBoard();
    print(g.won);

    print(g.setO(1,1));
    g.showBoard();
    print(g.won);

    print(g.setX(0,1));
    g.showBoard();
    print(g.won);

    print(g.setO(1,0));
    g.showBoard();
    print(g.won);

    print(g.setX(1,2));
    g.showBoard();
    print(g.won);

    print(g.setO(2,0));
    g.showBoard();
    print(g.won);

    # Playing out of turn
    print(g.setO(0,2));
    g.showBoard();
    print(g.won);

    # Overwriting a position
    print(g.setX(2,0));
    g.showBoard();
    print(g.won);

    print(g.setX(0,2));
    g.showBoard();
    print(g.won);



if __name__ == '__main__':
    main()

Readme

XO

A Python class for a game of X and O.

To use the class:

import xo game=xo.xo();

game.showBoard()

This displays:

  | X | 0
0 | 0 | X
0 |   | X

game.setX(<posx>,<posy>)

This sets 'X' at a certain position. If successful, returns 0; else -1

game.setO(<posx>,<posy>)

This sets 'O' at a certain position. If successful, returns 0; else -1

game.won

After a move, this variable will be True if the last player who played has won. Else, it is False.

Updated Version

Followup question

\$\endgroup\$
5
\$\begingroup\$

Short answer: not terribly well! It's a good start (the class is a sensible idea and if __name__ == '__main__': is a good way to run the code), but you have made a couple of mistakes. Here are a few pointers.


Style

Python has a style guide, which you should read and follow. A few highlights:

  • Naming: classes should be CamelCase and function/method/attribute names should be lowercase_with_underscores.
  • Whitespace: you should put spaces around operators, e.g. self.board = [...] and after commas (e.g. self.board = [[0, 0, 0], ...]).
  • Semicolons: just don't!

Documentation

It's good to have a README, although it's not clear what format it's in, but much better is to include docstrings for modules, classes, methods and functions, so that the script has documentation built right into it. This can also be used to generate formatted documentation (see e.g. Sphinx) and by your IDE to provide tooltips and even e.g. type hinting.


DRY

Or "don't repeat yourself". For example, note that setX and setO are almost exactly the same; you could refactor this to a single method which is called by the other methods as appropriate.

You could also significantly simplify main by using loops to reduce repetition, e.g.:

def main():
    print("Hello")
    g = xo()
    g.showBoard()
    now, nxt = g.setX, g.setO
    for x, y in [(2, 2), (1, 1), ...]:
        print(now(x, y))
        g.showBoard()
        print(g.won)
        now, nxt = nxt, now

"Magic numbers"

Like 1 and 2 (for X and O) and -1 and -2 (not clear) are best avoided - they don't provide any useful information to the reader and make refactoring very difficult. Instead, use named "constants", e.g.:

PLAYER_O = 1
PLAYER_X = 2

Then the code becomes clearer, e.g. self.win(PLAYER_X) tells you what's actually happening.


Naming

As well as the styling of the names, it's worth thinking about their meaning, too. It's best to avoid contractions (e.g. self.symbols is clearer than self.sym, and I've no idea what e.g. modResX really means). Better names means more readable code, which is a key aim of Python.


Functionality

  • It's conventional for a method to either change state and return None, or to return something but not change state - win returns something and changes state (although the return value is ignored anyway).
  • Rather than showBoard, I would be inclined to implement a __str__ method that returns the "friendly" representation of the board; then you can use print(g) rather than g.showBoard(). If you do retain the current functionality, note that again there's no need for a return value that's never used.
  • 0 is the default first argument to range, so you can write simply e.g. for i in range(3):, but note that it's neater to iterate over the board itself (i.e. for row in self.board:, etc.).
\$\endgroup\$
  • \$\begingroup\$ Thank you for the detailed comments. I am fixing the code. I'll post it once it is done. About "DRY" - My thoughts were that for a small function, calling another function involves pushing context to stack and popping it back on return may cause degradation of performance. So, I chose to rewrite most of it. Is there a flaw in my reasoning? I would however like to clean up win. \$\endgroup\$ – Lord Loh. Sep 2 '15 at 17:50
  • 1
    \$\begingroup\$ It might be slightly less performant, but if that's a big issue you're probably using the wrong language to start with! It's unlikely to be a bottleneck, anyway. \$\endgroup\$ – jonrsharpe Sep 2 '15 at 17:52
  • \$\begingroup\$ pastebin.com/ewkVJDW6 is an updated version. \$\endgroup\$ – Lord Loh. Sep 10 '15 at 19:09
  • \$\begingroup\$ @LordLoh. If you'd like a follow-up review, see meta.codereview.stackexchange.com/q/1065/32391 \$\endgroup\$ – jonrsharpe Sep 12 '15 at 8:05
  • \$\begingroup\$ Thank you. I have posted a follow up question - codereview.stackexchange.com/questions/104946/… \$\endgroup\$ – Lord Loh. Sep 17 '15 at 18:35

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service, privacy policy and cookie policy

Not the answer you're looking for? Browse other questions tagged or ask your own question.