8
\$\begingroup\$

Here's my solution to the Chess Dictionary Validator project in Automate the boring stuff by Al Sweigart. I'd love to know how I can improve this code so that it's more efficient. Thank you

Function accepts a dictionary that represents a chessboard. Eg: {'1h': 'bking', '6c': 'wqueen', '2g': 'bbishop', '5h': 'bqueen', '3e': 'wking'} . A valid board will have exactly one black king and exactly one white king. Each player can only have at most 16 pieces, at most 8 pawns, and all pieces must be on a valid space from '1a' to '8h'; that is, a piece can’t be on space '9z'. The piece names begin with either a 'w' or 'b' to represent white or black, followed by 'pawn', 'knight', 'bishop', 'rook', 'queen', or 'king'.

def isValidChessboard(chessboard): # function accept a dict #create a dictionary with all the pieces of each color as the key # and the valid starting number of the pieces as value pieces = {'pawn': 8,'knight': 2,'bishop': 2,'rook': 2,'queen': 1,'king': 1} colors = ['w','b'] all_pieces = {} #the valid dict to compare pieces and quantities to for color in colors: for p, num in pieces.items(): all_pieces[color + p] = num all_pieces[''] = 0 #no piece is valid if empty quare current_pieces = list(chessboard.values()) #get a list of all the pieces present on the chessboard def is_valid_p(current_pieces): #accepts a list, checking if the pieces of the chessboard are valid if 'bking' not in current_pieces or 'wking' not in current_pieces: return False for c_p in current_pieces: #checking if the pieces' names are valid if c_p not in all_pieces: return False for p in all_pieces: if p in current_pieces and p != '': #looping through all the valid p in valid dict, if p is present #on the chessboard, count and compare to the valid number of the piece if current_pieces.count(p) > all_pieces[p]: return False return True valid = True for position in chessboard: #checking coordinates of the square if len(position) != 2: valid = False break elif int(position[0]) not in range(1, 9) or position[1] not in ['a','b','c','d','e','f','g','h']: valid = False break else: if not is_valid_p(current_pieces): #checking the pieces itself valid = False if not valid: print("Your board is invalid") else: print("Your board is valid")
\$\endgroup\$
9
  • 4
    \$\begingroup\$ Your indentation seems off. Considering indentation is important for how Python interprets code, please double-check and fix where necessary. See the editing help for pointers. \$\endgroup\$ Commented May 6, 2023 at 21:53
  • \$\begingroup\$ I fixed it, kinda. Everything should be nested under def isValidChessboard(chessboard) but whenever I paste my code in here, the indentation becomes off. Thank you \$\endgroup\$ Commented May 6, 2023 at 22:20
  • 2
    \$\begingroup\$ Stack Overflow handles tabs differently than most editors; you may need to do something like emacs M-x untabify \$\endgroup\$ Commented May 7, 2023 at 14:57
  • \$\begingroup\$ You seem to have omitted some validation rules. For example, two pieces cannot occupy the same square, a pawn cannot be on row 1 or 8. \$\endgroup\$ Commented May 7, 2023 at 16:21
  • 2
    \$\begingroup\$ And of course, full information about the position also involves information about history (who has castled, is a pawn en-passent); and there are positions that are in some sense legal but not reachable, like having pawns on g2, h2, and h3. \$\endgroup\$ Commented May 8, 2023 at 6:59

2 Answers 2

Reset to default
8
\$\begingroup\$

Organization

It's a bit difficult to follow the logic of your function due to the large inner function is_valid_p() cutting it in half. Plus, most of the lines above this inner function are only used within it. So, I would move the inner function outside to make it more clear where work is being done.

def is_valid_p(current_pieces): #create a dictionary with all the pieces of each color as the key # and the valid starting number of the pieces as value pieces = {'pawn': 8,'knight': 2,'bishop': 2,'rook': 2,'queen': 1,'king': 1} colors = ['w','b'] all_pieces = {} #the valid dict to compare pieces and quantities to for color in colors: for p, num in pieces.items(): all_pieces[color + p] = num all_pieces[''] = 0 #no piece is valid if empty quare #accepts a list, checking if the pieces of the chessboard are valid if 'bking' not in current_pieces or 'wking' not in current_pieces: return False for c_p in current_pieces: #checking if the pieces' names are valid if c_p not in all_pieces: return False for p in all_pieces: if p in current_pieces and p != '': #looping through all the valid p in valid dict, if p is present #on the chessboard, count and compare to the valid number of the piece if current_pieces.count(p) > all_pieces[p]: return False return True def isValidChessboard(chessboard): # function accept a dict current_pieces = list(chessboard.values()) #get a list of all the pieces present on the chessboard valid = True for position in chessboard: #checking coordinates of the square if len(position) != 2: valid = False break elif int(position[0]) not in range(1, 9) or position[1] not in ['a','b','c','d','e','f','g','h']: valid = False break else: if not is_valid_p(current_pieces): #checking the pieces itself valid = False if not valid: print("Your board is invalid") else: print("Your board is valid")

Next, the logic of the isValidChessboard() can be cleaned up by removing the print() statements and having the function return True or False (most functions and methods that start with is___() return boolean values). Instead of using value to store the result, we can return once we know the answer.

def isValidChessboard(chessboard): # function accept a dict current_pieces = list(chessboard.values()) #get a list of all the pieces present on the chessboard for position in chessboard: #checking coordinates of the square if len(position) != 2: return False elif int(position[0]) not in range(1, 9) or position[1] not in ['a','b','c','d','e','f','g','h']: return False else: if not is_valid_p(current_pieces): #checking the pieces itself return False return True if not isValidChessboard(chessboard): print("Your board is invalid") else: print("Your board is valid")

Some last bits:

  • Since there's no more break within this for loop, the else clause will always happen, so let's save some indentation.
  • Instead of a list of letters for the valid files, we can use a str to save space.
  • Let's move current_pieces near to where it's used. (Side note: .values() already returns a list, so there's no need to convert it.)
  • Finally, since it's the last thing the function checks, we can return the result of is_valid_p()
def isValidChessboard(chessboard): # function accept a dict for position in chessboard: #checking coordinates of the square if len(position) != 2: return False elif int(position[0]) not in range(1, 9) or position[1] not in 'abcdefgh': return False current_pieces = chessboard.values() return is_valid_p(current_pieces): #checking the pieces itself

Following the requirements of the task

There are three requirements on the number of pieces on the board:

  1. Exactly one white king and one black king.
  2. No more than 8 pawns of either color.
  3. No more than 16 pieces of either color.

There is no restriction on the number of any other piece. Because of pawn promotion, a pawn can be converted into any other non-king piece. It is legal for there to be 2, 3, 4, or even 9 white queens on the board (unlikely, but legal). So, the line where you check the number of each type of piece if current_pieces.count(p) > all_pieces[p]: in is_valid_p() is incorrect.

First, let's change the pieces variable to a list and handle the counts later.

def is_valid_p(current_pieces): #create a dictionary with all the pieces of each color as the key # and the valid starting number of the pieces as value pieces = ['pawn', 'knight', 'bishop', 'rook', 'queen', 'king'] colors = ['w', 'b'] all_pieces = [] for color in colors: for p in pieces: all_pieces.append(color + p) all_pieces.append('') #no piece is valid if empty quare #accepts a list, checking if the pieces of the chessboard are valid if 'bking' not in current_pieces or 'wking' not in current_pieces: return False for c_p in current_pieces: #checking if the pieces' names are valid if c_p not in all_pieces: return False # Still needs fixing for p in all_pieces: if p in current_pieces and p != '': #looping through all the valid p in valid dict, if p is present #on the chessboard, count and compare to the valid number of the piece if current_pieces.count(p) > all_pieces[p]: return False return True

At the point where we reach the comment # Still needs fixing, we know that there is one white king and one black king, and that all of the piece names are valid. So, we can use simple counts on the piece list to check pawns and other pieces. The task description doesn't mention blank squares, so I don't see a use for the inclusion of '' in all_pieces.

def isValidPieceSet(current_pieces): #create a dictionary with all the pieces of each color as the key # and the valid starting number of the pieces as value pieces = ['pawn', 'knight', 'bishop', 'rook', 'queen', 'king'] colors = ['w', 'b'] all_pieces = [] for color in colors: for p in pieces: all_pieces.append(color + p) #accepts a list, checking if the pieces of the chessboard are valid if 'bking' not in current_pieces or 'wking' not in current_pieces: return False for c_p in current_pieces: #checking if the pieces' names are valid if c_p not in all_pieces: return False if current_pieces.count('wpawn') > 8: return False if current_pieces.count('bpawn') > 8: return False # Count the white pieces if len([p for p in current_pieces if p.startswith('w')]) > 16: return False # Count the black pieces if len([p for p in current_pieces if p.startswith('b')]) > 16: return False return True

Let's put the script back together after giving is_valid_p() a clearer name and formatting to match the other function.

def isValidPieceSet(current_pieces): #create a dictionary with all the pieces of each color as the key # and the valid starting number of the pieces as value pieces = ['pawn', 'knight', 'bishop', 'rook', 'queen', 'king'] colors = ['w', 'b'] all_pieces = [] for color in colors: for p in pieces: all_pieces.append(color + p) #accepts a list, checking if the pieces of the chessboard are valid if 'bking' not in current_pieces or 'wking' not in current_pieces: return False for c_p in current_pieces: #checking if the pieces' names are valid if c_p not in all_pieces: return False if current_pieces.count('wpawn') > 8: return False if current_pieces.count('bpawn') > 8: return False # Count the white pieces if len([p for p in current_pieces if p.startswith('w')]) > 16: return False # Count the black pieces if len([p for p in current_pieces if p.startswith('b')]) > 16: return False return True def isValidChessboard(chessboard): # function accept a dict for position in chessboard: #checking coordinates of the square if len(position) != 2: return False elif int(position[0]) not in range(1, 9) or position[1] not in 'abcdefgh': return False current_pieces = chessboard.values() return isValidPieceSet(current_pieces): #checking the pieces itself if not isValidChessboard(chessboard): print("Your board is invalid") else: print("Your board is valid")

At this point, we have a script that's easy to read and easy to check for correctness. One further improvement would be to create a function for each requirement so that each function only does one thing, but that can come later.

One more note

The task description and this program both allow illegal board positions to be counted as valid: boards with 10 white queens, boards with pawns on the first and eighth ranks, boards with kings attacking each other, and many more. The reason for this is the problem of determining whether an arbitrary board position is legal is very hard. Such a program would have to implement all the rules of chess and then find a sequence of moves that results in the input board. Such a search could take literal millions of years. So, since this task is meant to be programming practice, the scope of the checks is restricted to what is easily programmable.

\$\endgroup\$
4
  • \$\begingroup\$ Perhaps worth mentioning that the stated checks (which your code correctly implements) aren't sufficient to detect some impossible boards. For example, it will accept a board with 15 white queens, as if all the starting pieces have morphed into queens, as well as promoting the pawns. If you have 16 total pieces+pawns, you can't have lost any, so you do need at least the starting number of each piece. It's non-obvious what the simplest way to check the accounting would be. It also doesn't rule out pawns that moved backward from their starting row (wpawn on 1[a-h], bpawn on 8[a-h]). \$\endgroup\$ Commented May 7, 2023 at 15:12
  • 1
    \$\begingroup\$ There might be other more subtle and complicate ways positions can be invalid, if that's supposed to mean reachable in a standard game. e.g. king buried behind pawns that haven't advanced, with all pieces+pawns still on the board (otherwise the other knight could have popped in to take the bishop, allowing the king to castle.) Or king(s) in check. But that would make things more complicated than desired for this exercise, unlike pawn rows. I guess being a dictionary rules out duplicate keys, two pieces on the same space, something I thought at first was conspicuously absent. \$\endgroup\$ Commented May 7, 2023 at 15:16
  • 1
    \$\begingroup\$ @PeterCordes You're right that my code misses some illegal positions. The only true way to check the legality of a board is to find a sequence of moves from the starting position that results in the input position. If there is no such sequence of moves, the position is illegal. Such a program. would take many times the universe's age to run. That's why the task and my program are restricted to a few easy checks. \$\endgroup\$ Commented May 7, 2023 at 19:09
  • 2
    \$\begingroup\$ Yeah, your program does specifically the checks specified in the problem statement in the question, which is a good thing. My point was that it's worth mentioning that those checks are incomplete, even if we're not considering positional stuff, if we're going to discuss why the OP's 2,'bishop': 2,'rook' etc. data isn't needed here. That information about chess actually would be used in a more accurate algorithm that's still simple and fast, just checking amount of material for each side. (And pawn rows, and maybe that bishops are on opposite colors if we can prove neither was captured.) \$\endgroup\$ Commented May 7, 2023 at 19:19
9
\$\begingroup\$

Your code is good, but there are a few improvements to be made:

First, I would implement a better way to check for certain illegal configurations:

You could check for valid squares like:

valid_numbers = "12345678" valid_letters = "abcdefgh" squares_are_valid = True for square in chessboard: if not (square[0] in valid_letters and square[1] in valid_numbers: squares_are_valid = False break

or

squares_are_valid = all([square[0] in "abcdefgh" and square[1] in "12345678" for square in chessboard])

Then, you could check for valid piece names like:

valid_piece_names = ["pawn", "knight", "bishop", "rook", "queen", "king"] piece_names_are_valid = True for piece in chessboard.values(): if not (piece[0] in "wb" and piece[1:] in valid_piece_names): piece_names_are_valid = False break

or again with list comprehension like:

valid_piece_names = ["pawn", "knight", "bishop", "rook", "queen", "king"] piece_names_are_valid = all([piece[0] in "wb" and piece[1:] in valid_piece_names for piece in chessboard.values()])

You could check for the piece requirements like:

pieces = list(chessboard.values()) kings_are_valid = pieces.count("wking") == 1 and pieces.count("bking") = 1 white_pieces_are_valid = len([piece for piece in pieces if piece[0] == "w"]) <= 16 black_pieces_are_valid = len([piece for piece in pieces if piece[0] == "b"]) <= 16 pawns_are_valid = pieces.count("wpawn") <= 8 and pieces.count("bpawn") <= 8

(you can do these with for loops like shown above if you want to as well.)

Other things:

  • You should be careful of your indentation, it seems off.
  • You might want to name your functions in snake case, as it is the convention for python. (is_valid_chess_board not isValidChessBoard.)
  • When commenting, you might want to put a space between the # and your comment, like: # this is a comment not #this is a comment.

Full Code:

def is_valid_chessboard(chessboard): valid_numbers = "12345678" valid_letters = "abcdefgh" valid_piece_names = ["pawn", "knight", "bishop", "rook", "queen", "king"] pieces = list(chessboard.values()) squares_are_valid = all([square[0] in valid_letters and square[1] in valid_numbers for square in chessboard]) piece_names_are_valid = all([piece[0] in "wb" and piece[1:] in valid_piece_names for piece in pieces]) # pieces kings_are_valid = pieces.count("wking") == 1 and pieces.count("bking") = 1 white_pieces_are_valid = len([piece for piece in pieces if piece[0] == "w"]) <= 16 black_pieces_are_valid = len([piece for piece in pieces if piece[0] == "b"]) <= 16 pawns_are_valid = pieces.count("wpawn") <= 8 and pieces.count("bpawn") <= 8 return all([ squares_are_valid, piece_names_are_valid, kings_are_valid, white_pieces_are_valid, black_pieces_are_valid, pawns_are_valid ])
\$\endgroup\$

You must log in to answer this question.

Start asking to get answers

Find the answer to your question by asking.

Ask question

Explore related questions

See similar questions with these tags.