20
\$\begingroup\$

I have programmed for 2 months, and I began writing a Chess game. I am a beginner programmer in Python, so please assess my code.

class Chess_Board:
    def __init__(self):
        self.board = self.create_board()

    def create_board(self):
        board_x=[]

        for x in range(8):
            board_y =[]
            for y in range(8):

                board_y.append('.')

            board_x.append(board_y)
        board_x[7][4] = 'K'
        board_x[7][3] = 'Q'
        board_x[7][2] = 'B'
        board_x[7][1] = 'N'
        board_x[7][0] = 'R'
        return board_x

class WHITE_KING(Chess_Board):
    def __init__(self):
        Chess_Board.__init__(self)
        self.position_x_WK = 7
        self.position_y_WK = 4
        self.symbol_WK = 'K'

    def move(self):
        while True:
            try:
                print ('give a x and y coordinate for WHITE KING')
                destination_x_WK = int(input())
                destination_y_WK = int(input())


                if self.board[destination_x_WK][destination_y_WK] == '.' :

                    if ( abs(self.position_x_WK-destination_x_WK) <2 and abs(self.position_y_WK-destination_y_WK) < 2 ):
                        self.board[self.position_x_WK][self.position_y_WK] = '.'
                        self.position_x_WK = destination_x_WK
                        self.position_y_WK = destination_y_WK
                        self.board[self.position_x_WK][self.position_y_WK] = self.symbol_WK

                        return self.board
                        break

                    else:
                        print ('your move is invalid, please choose cooridnates again')
                        continue

            except:
                pass


class WHITE_QUEEN(Chess_Board):
    def __init__(self):
        Chess_Board.__init__(self)
        self.position_x_WQ = 7
        self.position_y_WQ = 3
        self.symbol_WQ = 'Q'

    def move(self):
        while True:
            try:
                print ('give a x and y coordinate for WHITE QUEEN')
                destination_x_WQ = int(input())
                destination_y_WQ = int(input())


                if self.board[destination_x_WQ][destination_y_WQ] == '.' :

                    if (destination_x_WQ == self.position_x_WQ or destination_y_WQ==self.position_y_WQ or abs(self.position_x_WQ-destination_x_WQ) == abs(self.position_y_WQ-destination_y_WQ) ):
                        self.board[self.position_x_WQ][self.position_y_WQ] = '.'
                        self.position_x_WQ = destination_x_WQ
                        self.position_y_WQ = destination_y_WQ
                        self.board[self.position_x_WQ][self.position_y_WQ] = self.symbol_WQ

                        return self.board
                        break

                    else:
                        print ('your move is invalid, please choose cooridnates again')
                        continue

            except:
                pass

class WHITE_ROOK(Chess_Board):

    def __init__(self):
        Chess_Board.__init__(self)
        self.position_x_WR = 7
        self.position_y_WR = 0
        self.symbol_WR = 'R'

    def move(self):
        while True:
            try:
                print ('give a x and y coordinate for WHITE ROOK ')
                destination_x_WR = int(input())
                destination_y_WR = int(input())


                if self.board[destination_x_WR][destination_y_WR] == '.' :

                    if (destination_x_WR == self.position_x_WR or destination_y_WR==self.position_y_WR  ):
                        self.board[self.position_x_WR][self.position_y_WR] = '.'
                        self.position_x_WR = destination_x_WR
                        self.position_y_WR = destination_y_WR
                        self.board[self.position_x_WR][self.position_y_WR] = self.symbol_WR

                        return self.board
                        break

                    else:
                        print ('your move is invalid, please choose cooridnates again')
                        continue

            except:
                pass

class WHITE_BISHOP(Chess_Board):
    def __init__(self):
        Chess_Board.__init__(self)
        self.position_x_WB = 7
        self.position_y_WB = 2
        self.symbol_WB = 'B'

    def move(self):
        while True:
            try:
                print ('give a x and y coordinate for WHITE BISHOP')
                destination_x_WB = int(input())
                destination_y_WB = int(input())


                if self.board[destination_x_WB][destination_y_WB] == '.' :

                    if  abs(self.position_x_WB-destination_x_WB) == abs(self.position_y_WB-destination_y_WB) :
                        self.board[self.position_x_WB][self.position_y_WB] = '.'
                        self.position_x_WB = destination_x_WB
                        self.position_y_WB = destination_y_WB
                        self.board[self.position_x_WB][self.position_y_WB] = self.symbol_WB

                        return self.board
                        break

                    else:
                        print ('your move is invalid, please choose cooridnates again')
                        continue

            except:
                pass

class WHITE_KNIGHT(Chess_Board):
    def __init__(self):
        Chess_Board.__init__(self)
        self.position_x_WKN = 7
        self.position_y_WKN = 1
        self.symbol_WKN = 'N'

    def move(self):
        while True:
            try:
                print ('give a x and y coordinate for WHITE KNIGHT')
                destination_x_WKN = int(input())
                destination_y_WKN = int(input())


                if self.board[destination_x_WKN][destination_y_WKN] == '.' :

                    if abs(self.position_x_WKN-destination_x_WKN)**2 + abs(self.position_y_WKN-destination_y_WKN)**2 == 5 :
                        self.board[self.position_x_WKN][self.position_y_WKN] = '.'
                        self.position_x_WKN = destination_x_WKN
                        self.position_y_WKN = destination_y_WKN
                        self.board[self.position_x_WKN][self.position_y_WKN] = self.symbol_WKN

                        return self.board
                        break

                    else:
                        print ('your move is invalid, please choose cooridnates again')
                        continue

            except:
                pass

class Engine(Chess_Board):

    def __init__(self):
        WHITE_KING.__init__(self)
        WHITE_QUEEN.__init__(self)
        WHITE_ROOK.__init__(self)
        WHITE_BISHOP.__init__(self)
        WHITE_KNIGHT.__init__(self)
        Chess_Board.__init__(self)

    def play(self):
        print('Please write what figure you choose to move: white_king, white_queen, white_rook, white_bishop'
              'or white knight')

        while True:
            choice=str(input())
            if  choice == 'white_king':
                WHITE_KING.move(self)
                break
            elif  choice == 'white_queen':
                WHITE_QUEEN.move(self)
                break
            elif  choice == 'white_bishop':
                WHITE_BISHOP.move(self)
                break
            elif  choice == 'white_knight':
                WHITE_KNIGHT.move(self)
                break
            elif  choice == 'white_rook':
                WHITE_ROOK.move(self)
                break
            else:
                print ('please choose again')


    def display(self):
        for i in range (8):
            for j in range (8):
                print (self.board[i][j], end=' ')
            print ()


    c_engine = Engine()
    c_engine.display()
    c_engine.play()
    c_engine.display()
\$\endgroup\$
2
  • \$\begingroup\$ It is actually easier to just paste the code as it is and then formatting it as source code by selecting it and either pressing Ctrl-k or clicking the {} button than to add four spaces in front of all classes. \$\endgroup\$
    – mkrieger1
    Commented Aug 21, 2015 at 16:36
  • \$\begingroup\$ I saw that this is still flagged as active; I have added an answer with updates and further suggestions in the follow-up question codereview.stackexchange.com/questions/104467/… \$\endgroup\$
    – Max
    Commented Dec 15, 2021 at 14:53

4 Answers 4

15
\$\begingroup\$

This is a lot of work, and I don't have a lot of time, but I thought I'd throw in my two cents.

So, here's what I've got for you:

The Good

  • Your models are nicely formed. More than a data store, they actually do stuff. This is good practise.
  • You've compartmentalised the code into objects that are easy to read and follow. Good job.

The Bad

  • You're violating pep8 all over the place. This is the gold standard for Python development so you really should conform your code to it. Specifically some of the more glaring violations:
    • Your lines exceed 80 characters a lot
    • It's print(, not print (
    • Operators like = are supposed to be surrounded by spaces unless used in a keyword argument, in which case there shouldn't be any spaces.
    • Your class names are in ALL_CAPS. Don't do that. All caps is meant for constants only.
    • if statements should end with a : with no spaces to the left or right.
  • Your variable names need some work
    • They violate pep8 since you're using all caps in some in whole or in part.
    • They're sometimes not using words. WKN means nothing to someone who didn't write the code.
  • Your class names are all caps and have underscores. Again, this is a violation of pep8.
  • The end of your file has raw logic not wrapped in if __name__ == "__main__":. This means that if someone were to import your file, your program would actually run. This is very bad form.
  • Too much vertical space. Again, pep8 dictates that there's one blank line before every method, two before a class.

For the most part, it's all pep8 stuff, so that's good news. I didn't actually run the program though, so there may be more that I've missed. I like to go for style & readability first anyway.

You may also want to consider breaking your code out into separate files for readability and to keep the file you're working on short and simple to follow.

\$\endgroup\$
0
20
\$\begingroup\$

There are a few traps you're getting yourself into a bit.

  • By definition your white rook starts at a specific spot. What are you going to do for the other rook?

  • Also you're using class inheritance White_Knight(chess_board) in a funny way. Please read this quick chapter. In short, your implying that your white knight IS a chess board. But your white knight IS a piece and so perhaps you should have a piece class and have white knight inherit from that. Your chess board HAS pieces, so your structure should reflect that.

  • You're limiting yourself to a class which specifies color. You should just have a knight class so that all 4 knights (on each side) can use that same class.

  • When I tell it my move I shouldn't have to tell it "Move white knight" because it should already know its my turn, so the white is unnecessary. And the same time that doesn't tell us WHICH white knight to even move, so it is too little and too much information at the same time. One way to have people input moves would be using standard chess notation, so you can read up more on that and see if you want to use it.

  • Consider using inputs to your classes. Suppose you had a class like knight. You could initialize it with something like board_x[7][1] = Knight(pos_y=7, pos_x=1, team='white') that way white and position are just properties of a broader knight class and you have a class that can be re-used instead of making a class specific to that one specific knight on the board.

\$\endgroup\$
2
  • \$\begingroup\$ Thanks for answer. I am going to follow your advices. It will help me a lot. \$\endgroup\$
    – Rafail
    Commented Aug 21, 2015 at 16:20
  • \$\begingroup\$ I share the same idea related to separating board class and piece class. In such a case how should a move be implemented? The two classes are not "communicating" so, after the validity of the move has been checked the placement of the piece on the board should happen independently (and not in move method), right? \$\endgroup\$
    – Pitto
    Commented May 3, 2016 at 21:24
10
\$\begingroup\$

First, you can make your empty board much faster. For a start, you can create a list of 8 '.'s with the multiplication operator.

board = ['.'] * 8

And you can make it have the 2 axes with a thing called a list comprehension. It's basically a shorthand for generating a list with a for loop.:

board = [['.']*8 for _ in range(8)]

Using _ in this loop is just a Python style for saying that the variable doesn't matter, we really just want to iterate 8 times.

I also don't recommend naming your board board_x as that implies it's specifically only the x axis, when it contains both. I would also say there's no need to separate __init__ from create_board

def __init__(self):
    """Initialise a new chess board."""

    self.board = [['.']*8 for _ in range(8)]

    board[7][4] = 'K'
    board[7][3] = 'Q'
    board[7][2] = 'B'
    board[7][1] = 'N'
    board[7][0] = 'R'

Also confusingly you seem to only initialise some pieces of the board. If this is intentional, you should explain why in a comment and not leave people to guess.


Now the White King. I agree about the inheritance being funny, I also don't see why he would need to inherit and create a separate copy of the board. This sounds like it would only create issues.

For parameters, I think you should name them shorter and don't need to be as explicit. People will know what WhiteKing.x is, you don't need position and WK around it. Likewise you can cut down to symbol as your last name.

I would also suggest editing the chess board when creating this piece. This is when you should insert the King into the board as it means you're essentially creating and placing him on the board. That does also mean you'd need to pass the chessboard as a parameter, but that's good practice anyway so that the King is linked to the board, rather than having an inheritance to join them.

def __init__(self, chessboard):
    self.x = 7
    self.y = 4
    self.symbol = 'K'
    chessboard[self.x][self.y] = self.symbol

Note that you could make this cleverer if you added a method to the board that would take a piece and two co-ordinates as a parameter and handle their movement. Something like this:

class Chess_Board:

    ...

    def move(self, piece, x, y):
        #Reset the old spot to be empty then update the new spot
        self.board[piece.x][piece.y] = '.'
        self.board[x][y] = piece.symbol

        #Now that it's moved, update the piece itself
        piece.x = x
        piece.y = y

The reason this is a good idea is that you can now have the board check if a move is actually allowed for the board. For example, not going off the edges or not trying to take a king (in the latter case it should instead be presenting as either a check or checkmate), while the piece itself can handle telling the player that it can't move that way (ie trying to get a king to travel more than one square at a time).


Back to the King, where you did create a move function. First please don't use a bare try: except:. If you are generating errors you should know exactly what the error is and where it can happen, and account specifically for that issue. A bare try: except: wouldn't even tell you if you made a typo and would leave the error unchecked.

I'm guessing you want this to ignore either input errors or IndexErrors, but in both cases they ought to be handled, not ignored. So for input, I'd set up a while loop that breaks when two valid inputs are supplied. Like this:

while True:
    try:
        print ('Give a x and y coordinate for WHITE KING')
        movex = int(input())
        movey = int(input())
        break
    except ValueError:
        print ('Co-ordinates can be only be an integer')

(again I used shorter but still clear variable names)
A ValueError is what's raised if int() is unable to parse a number from what's supplied. I also reccomend giving some feedback to the user about what they've done:

    print ("White King to {} {}...".format(movex, movey))

str.format is a good method for inserting variables into a string, I can explainfurther if you're not familiar with it.

The tests you do here for what's in the destination is mostly stuff I'd move to Chess_Board as I suggested earlier. However I'd keep the checks to ensure that the King specifically can move the amount the user has suggested. Your test for the king's movement seems generally good to me, except I'd flip to check if it is moving more than 1, and use or since that way it only needs to check if it's moved more than 1 in either case. However, you did forget to check if the King isn't being moved, so I added that too.

        if (abs(self.x - movex) > 1 or abs(self.y - movey) > 1 or
                (self.x == movex and self.y == movey)):

In order to allow the user to input a new number you can actually just have the Chess_Board.move function return a boolean, True if the move was allowed but otherwise False. Here's how this whole method would work:

def move(self):
    while True:
        while True:
            try:
                print ('Give a x and y coordinate for WHITE KING')
                movex = int(input())
                movey = int(input())
                break
            except ValueError:
                print ('Co-ordinates can be only be an integer')

        if (abs(self.x - movex) > 1 or abs(self.y - movey) > 1 or
                (self.x == movex and self.y == movey)):
            print ('The king cannot perform that move, please choose co-ordinates again.')
            continue

        print ('White King to {} {}...'.format(movex, movey))
        if chessboard.move(self, movex, movey):
            break # Moved successfully

        print ('Your move is invalid for this board, please choose co-ordinates again.')
        # You could have a more specific message about not placing the King in check.

Even if you don't take these exact suggestions, I think it's worth thinking more about what a class is and should do. Why should the King check if he's allowed to go to square 8, 4. That's the board's job!

\$\endgroup\$
1
  • \$\begingroup\$ There are lots of good points in this, but in terms of OOP, King inherits Piece, which has a Move function. All pieces override MoveDirectionRules, as well as their Attribute which is Colour. Colour defines which StartRow the piece begins at, but the Class definition of each Piece defines which Column the piece initialises at. The Move() function above would read the piece location, and movement rules, and build a map of possible destinations for the piece. If the requested destination isn't inside the map, it is should raise an InvalidMove() custom exception and have the user try again. \$\endgroup\$
    – C. Harley
    Commented Aug 2, 2018 at 13:40
3
\$\begingroup\$

The first thing I thought when I saw this was: "Why are there so many classes in one file?". The first thing I would do is split this code up into multiple files. (Such as: the piece classes should go into their own file). Next thing: Use:

if __name__ == "__main__":

to start it. The way it is now, if someone imported it as a module, it would run. The if name == "main": makes sure that the code only runs if the actual file is run.

Also: use PEP-8.

Just throwing in my $00.02 here.

\$\endgroup\$
1
  • 1
    \$\begingroup\$ I think not "more files" but "less code", esp. avoiding code duplication, would be the way to go. All pieces could be a subclass of a class Piece with essentially the same content. But not starting positions: these can be coded in a single FEN string - half a line for the whole board, not 5 lines for each piece! (Also consider extension to other 960 starting positions, and importing arbitrary positions!) Please see the follow-up question for a more developed answer: codereview.stackexchange.com/questions/104467/… \$\endgroup\$
    – Max
    Commented Dec 18, 2021 at 3:06

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