Skip to main content
Code Review

Return to Answer

Add link to further answer
Source Link
AlexV
  • 7.4k
  • 2
  • 24
  • 47

There is a lot left to discuss. I have not yet touched on the chess pieces or their interaction with the board. Maybe future me (Edit: seems like it did ) or an other member of the community will further elaborate on that topic.

There is a lot left to discuss. I have not yet touched on the chess pieces or their interaction with the board. Maybe future me or an other member of the community will further elaborate on that topic.

There is a lot left to discuss. I have not yet touched on the chess pieces or their interaction with the board. Maybe future me (Edit: seems like it did ) or an other member of the community will further elaborate on that topic.

added 1 character in body
Source Link
AlexV
  • 7.4k
  • 2
  • 24
  • 47

The major changes are that board and b_len are notnow instance attributes instead of class attributes. Now you have to explictelyexplicitly create an instance of the board, using board = Chessboard("default") or the like, before being able to use it. This will make all the if self.board != "UNITIALIZED": checks obsolote. Apart from that you could now have multiple boards to play with if you like. The __init__ constructor now more or less replaced new_board from the original implementation. As a bonus features I added an exception to signal the failure to create the board. This will also help you to detect the failure to do so programatically later on. You can read more about exceptions and exception handling in the documentation.

The major changes are that board and b_len are not instance attributes instead of class attributes. Now you have to explictely create an instance of the board using board = Chessboard("default") or the like before being able to use it. This will make all the if self.board != "UNITIALIZED": checks obsolote. Apart from that you could now have multiple boards to play with if you like. The __init__ constructor now more or less replaced new_board from the original implementation. As a bonus features I added an exception to signal the failure to create the board. This will also help you to detect the failure to do so programatically later on. You can read more about exceptions and exception handling in the documentation.

The major changes are that board and b_len are now instance attributes instead of class attributes. Now you have to explicitly create an instance of the board, using board = Chessboard("default") or the like, before being able to use it. This will make all the if self.board != "UNITIALIZED": checks obsolote. Apart from that you could now have multiple boards to play with if you like. The __init__ constructor now more or less replaced new_board from the original implementation. As a bonus features I added an exception to signal the failure to create the board. This will also help you to detect the failure to do so programatically later on. You can read more about exceptions and exception handling in the documentation.

Source Link
AlexV
  • 7.4k
  • 2
  • 24
  • 47

Style

The overall style is good and mostly follows the official style guide. Since you have a lot of code and the scope will likely expand even more, I would highly recommend using an automatic style checking tool like pylint, flake8, or the like. There are a lot of Python IDEs which integrate nicely with those style checkers (Visual Studio Code, PyCharm, Spyder, ...) and can even annotate (some also autofix) your code while you write. This will help you to keep a consistent code style even on a larger scope.

The main thing I would critique about the code's style is the sometimes inconsistent usage of vertical whitespace. The usual conventions is to have at most one line of vertical whitespace within functions, methods, and classes. Individual functions and classes should be seperated by two blank lines. There are also usually two blank lines following the imports.

Overall structure

The overall structure is quite good. Nevertheless I would actually recommend splitting the file into three files. A file for the chess pieces, one for the config/board and an application script which ties everything together. In order to pull this off, the classes might need a little bit of work, but we will come to that later.

The application script should also likely make use of the infamous if __name__ == "__main__":, to make it clear which parts are actually supposed to be run as script.

The classes

Config

This class is actually the piece of code I like the least. First, I don't like the name. It is generic and IMHO does not really fit what the class is about. While looking through the code, it becomes more and more apparent that Config is the actual chessboard. So why not call it just like that, Chessboard?

The second thing I don't like about this class is the way that classmethods are used. I can see why it might look appealing to define everything as attributes and methods of the class in order to avoid having to pass around the board to the chesspieces. But I think there are better alternatives to do this.

Some parts of it can stay as class attributes such as types, white_pieces, and black_pieces1, although I would capitalize their names in order to make it clear that these are supposed to be seen as constants. Since you are also using letters in a few places, it might be also be legetimate to have them as class attribute, though I would at least prepend the name with an _ to make it clear that this is an implementation detail and not supposed to be used by an ordinary user.

So what could the new class look like?

class Chessboard:
 """The chessboard to play on
 The board can have different sizes, the default one has 8x8 fields. There
 is a list of predefined sizes in Chessboard.TYPES, and you can request a
 custom size by passing "custom<n>", where <n> is should be convertible to
 an integer between 1 and 26, as an argument to the constructor.
 """
 TYPES = {
 'min': 1, 'miniature': 3, 'small': 5, 'default': 8,
 'extended': 11, 'large': 15, 'massive': 20, 'max': 26
 }
 WHITE_PIECES = {
 'Pawn': "♙", 'Rook': "♖", 'Knight': "♘",
 'Bishop': "♗", 'King': "♔", 'Queen': "♕"
 }
 BLACK_PIECES = {
 'Pawn': "♟", 'Rook': "♜", 'Knight': "♞", 
 'Bishop': "♝",'King': "♚",'Queen': "♛"
 }
 _LETTERS = string.ascii_lowercase # this is basically'abcdefghijklmnopqrstuvwxyz'
 def __init__(self, board_type="default", show=False):
 self.b_len = None
 if board_type in self.TYPES:
 self.b_len = self.TYPES[board_type]
 elif board_type.startswith("custom"):
 try:
 self.b_len = int(board_type.replace('custom', '').strip())
 except ValueError:
 pass
 if self.b_len is None:
 raise ValueError(f"Cannot create board for type '{board_type}'")
 if self.b_len < 1 or self.b_len > 26:
 raise ValueError(f"The board size has to be between 1 and 26, was {self.b_len}")
 
 self.board = self.board = [['___' for _ in range(self.b_len)]
 for _ in range(self.b_len)]
 if show:
 self.print_board()

The major changes are that board and b_len are not instance attributes instead of class attributes. Now you have to explictely create an instance of the board using board = Chessboard("default") or the like before being able to use it. This will make all the if self.board != "UNITIALIZED": checks obsolote. Apart from that you could now have multiple boards to play with if you like. The __init__ constructor now more or less replaced new_board from the original implementation. As a bonus features I added an exception to signal the failure to create the board. This will also help you to detect the failure to do so programatically later on. You can read more about exceptions and exception handling in the documentation.

Next up on the list in this class is print_board. Same suggestion as above: no class method needed, make it an instance method. As mentioned above this lets you get rid of the initial check. One level of nesting gone. The method itself is not easily readable and left me thinking for a few moments. You define a helper function, printl as follows:

def printl():
 if len(str(cls.b_len)) == 2:
 print(' ', end='')
 for x in range(cls.b_len):
 print(' '*6 + f'{cls.letters[x]}', end='')
 print('\n')

After staring at it for a while I realized that this is the part where the "coordinate letters" are printed, and that you are trying to determine if you need an extra whitespace upfront to have it aligned correctly for boards with more than 10 fields. All of this can be done quite a bit shorter, for example:

def print_letters():
 print((" " if self.b_len >= 10 else "") 
 + "".join(f"{letter:>7}" for letter in self._LETTERS[:self.b_len]))

Let's break this down: (" " if n >= 10 else "") does add that extra leading whitespace for larger boards, f"{letter:>7}" prints letter right-aligned in a field of width 7 (6+the letter itself), and the letters are taken from self._LETTERS. All these formatted letters are then joined together and printed.

The rest of the code can more or less stay like this. If you are willing to print the numbers with a width of 2 for boards with a size smaller than 10, you can save yourself some code in the function above and in the line where the board is printed. I also advocate to avoid hardcoding additional leading and trailing newlines, since this takes control away from the user on how to have it printed to the console.

def print_board(self, leading=2, trailing=4):
 """Print the board to the console
 The number of leading and trailing newlines can be configured 
 and is 2, respectively 4 by default
 """
 def print_letters():
 print(" " + "".join(f"{letter:>7}" for letter in self._LETTERS[:self.b_len]))
 print("\n"*leading, end="") # end="" is to avoid newlines for 0
 print_letters()
 for i in range(self.b_len):
 print(
 f'{self.b_len-i:>2} {self.board[i]} {self.b_len-i:>2}\n'
 )
 print_letters()
 print("\n"*trailing, end="")

If you want to prettify the print any further, you should have a look at the wide range of Unicode box characters.

The modifications to the other functions are mostly straightforward

def tile_convert(self, x, display_tile=False):
 if not display_tile:
 if isinstance(x, str):
 return self._LETTERS.index(x)
 else:
 return self._LETTERS[x]
 else: # display_tile converts the letter in {letter}{number} to a number
 return self.b_len - int(x)
def l_num_to_coord(self, pos):
 return self.b_len - int(pos[1]), int(self.tile_convert(pos[0]))
def coord_to_tile(self, x, y):
 return f'{self.tile_convert(x)}{self.tile_convert(y, True)}'
def convert_color(self, color):
 if color == 'White':
 return 'b'
 if color == "Black":
 return 'w'

All of them would greatly benefit from a little bit of documentation, since their function is not always obvious from the name.


There is a lot left to discuss. I have not yet touched on the chess pieces or their interaction with the board. Maybe future me or an other member of the community will further elaborate on that topic.

Until then: Happy coding!


1 Funny sidenote: I tested your program in a console with a black background. Under these circumstances the black pieces do actually look more like white pieces and vice versa.

lang-py

AltStyle によって変換されたページ (->オリジナル) /