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:
- Exactly one white king and one black king.
- No more than 8 pawns of either color.
- 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.
M-x untabify\$\endgroup\$