6
\$\begingroup\$

I recently started learning python and came up with this solution to the "Chess Dictionary Validator" exercise which is given in the end of chapter 5 in the book "Automate the Boring Stuff" by Al Sweigart:

In this chapter, we used the dictionary value {'1h': 'bking', '6c': 'wqueen', '2g': 'bbishop', '5h': 'bqueen', '3e': 'wking'} to represent a chess board. Write a function named isValidChessBoard() that takes a dictionary argument and returns True or False depending on if the board is valid.

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'. This function should detect when a bug has resulted in an improper chess board.

What suggestions do you have in order to improve this solution?

# prerequisites
# board: a-h, 1-8
# pieces: max 16 per player
# 8x "pawn", 
# "knight",
# "bishop",
# "rook", 
# 1x "king", 
# 1x "queen"

#mainfunction 
def chessCheck(brett):
    # empty dict, should later collect the number of all figures (no matter if w or s)
    valueCheck = {}
    stri =""
    #list of valid fields
    validFields = ["a","b","c","d","e","f","g","h"]
    #if all is valid, should remain == 0, if not then > 0
    wrongSettings = 0

    #first loop through keys from dictionary 
    for k in brett.keys():
        stri = str(k)
        #go through each key as a string in subloop
        for x in stri:
            #here the first part of the key value (numbers) is checked
            if int(x.isdecimal() ):
                xpos = int(x)
                #check if valid 
                if xpos <= 8:
                    pass
                else:
                    #if no valid pos, then valid count becomes greater than 0
                    wrongSettings += 1
            
            else:
                #here it is checked whether letter part of string is NOT present in sample list
                # if not present -> error counter high
                if x not in validFields:
                    wrongSettings += 1

    #loopt trough item pairs from dict 
    for k,v in brett.items():
        # adds keys to new dict, than count for that key
        valueCheck.setdefault(v, 0)
        valueCheck[v] = valueCheck[v] +1

    # check after creating the frequency list of the figures, whether conditions are met
    # separate created dictionary into black and white
    blackChess = {}
    whiteChess = {}
    # loop through count dictionary from above
    for k,v in valueCheck.items():
        #if "key" begins with "w" -> in white dict
        if str(k).startswith("w"):
            whiteChess[k] = v
        #otherwise fill black dict at key position with value
        elif str(k).startswith("b"):
            blackChess[k] = v

    # check here NUMBER of chess pieces PER player
    # apply function, returned number, save in both vars
    wpieces = countFigures(whiteChess)
    bpieces = countFigures(blackChess)

    #check here if LESS than 16 figures per player
    if wpieces > 16:
        print("Too many pieces")
        wrongSettings += 1

    elif bpieces > 16:
        wrongSettings += 1

    #check for white with test function
    #update number of errors 1
    wrongSettings = figureChecks(whiteChess, wrongSettings)
    #anz fehler updaten part 2
    wrongSettings = figureChecks(blackChess, wrongSettings)
    
    #checks at the end how counter stands for false condition
    if wrongSettings > 0:
        return False
    else:
        return True


#function to check number of pieces per player
def figureChecks(figureDict,wrongcounter):
        for k,v in figureDict.items():
            # pawns max 8
            if str(k) == "wpawn" and v > 8:
                print("Too many pawns white")
                wrongcounter += 1
            elif str(k) =="wking" and v > 1:  #king
                print("Too many kings white")
                wrongcounter +=1
            elif str(k) =="wqueen" and v > 1:   #queen
                print("Too many quuens white")
                wrongcounter +=1
        return wrongcounter    


#function for number of figures for black and white count
def countFigures(player):
    piece = 0
    for i in player.values():
        piece = piece + int(i)
    return piece
        

# example dict

# Example 1: valid chessboard (each player has 16 pieces)
exp1 = {'1a': 'wrook', '1b': 'wknight', '1c': 'wbishop', '1d': 'wqueen', '1e': 'wking', '1f': 'wbishop', '1g': 'wknight', '1h': 'wrook',
 '2a': 'wpawn', '2b': 'wpawn', '2c': 'wpawn', '2d': 'wpawn', '2e': 'wpawn', '2f': 'wpawn', '2g': 'wpawn', '2h': 'wpawn',
 '8a': 'brook', '8b': 'bknight', '8c': 'bbishop', '8d': 'bqueen', '8e': 'bking', '8f': 'bbishop', '8g': 'bknight', '8h': 'brook',
 '7a': 'bpawn', '7b': 'bpawn', '7c': 'bpawn', '7d': 'bpawn', '7e': 'bpawn', '7f': 'bpawn', '7g': 'bpawn', '7h': 'bpawn'}



print(
chessCheck(exp1)
)
\$\endgroup\$
3
  • 1
    \$\begingroup\$ Is there any particular reason you count errors instead of having a binary variable keeping that "falseness state", or even just returning false the moment you see one issue? Also, while not in instructions, there are many other checks you would have to do to make sure this position is not obviously unreachable - like having 10 queens or pawns in say 1a or 8f. ((Note that proving position is unreachable is next to impossible)) \$\endgroup\$ Jan 17 at 8:04
  • \$\begingroup\$ In the beginning I wanted to use an empty list and append the error and later return it to have more detailed infos why the input format was wrong and then check the length of that list to see it failed or worked. Also I think you wouldn't be able to habe 10 queens on 1a because the latest entry for "1a" as a key with the value "wqueen" would overwride the earlier entries for that key in the dictionarry since double entries are not allowed? \$\endgroup\$
    – Ro14514
    Jan 17 at 8:56
  • \$\begingroup\$ I didn't mean 10 queens on 1a, I meant 10 queens in total. Or a single pawn anywhere in rows 1 or 8. \$\endgroup\$ Jan 17 at 9:04

3 Answers 3

10
\$\begingroup\$

signature

def chessCheck(brett):

Pep8 asks that you spell it chess_check.

It's unclear what a brett might be, and you chose not to annotate it with a type. You passed up the opportunity to add a function """docstring""" or even a # comment. This signature communicates much less to the reader than I was hoping it would.

vague comment

    # empty dict, should later collect the number of all figures (no matter if w or s)
    valueCheck = {}

Thank you for trying. I appreciate that you felt a comment was needed.

But I'm afraid I still don't know what value_check should hold. I think a "figure" is a chess piece, and it might be the dictionary key. But I don't know what "w" or "s" denotes. Maybe "w" or "b"? For white or black?

OIC, a subsequent comment "anz fehler updaten" reveals that author's Muttersprache is not English, wir verwenden hier Deutsch, gut. Ich lese weiß oder schwarz. Changing tongue mid-sentence can be disorienting.

terminology

    #list of valid fields
    validFields = ["a","b","c","d","e","f","g","h"]

Elide the comment, as it says nothing beyond what the code eloquently stated.

The usual terms from the Business Domain would be "rank" and "file". I imagine I should translate "field" to "file" (column) as I'm reading this?

inconsistent naming

        stri = str(k)

That's a bit jarring. Expected to see strk. Maybe the loop index used to be i?

It's unclear why introducing a temp var is a win over just using the original str() expression.

use pass only where needed

                if xpos <= 8:
                    pass
                else:
                    #if no valid pos, then valid count becomes greater than 0
                    wrongSettings += 1

No, please don't do that. Prefer to phrase it this way:

                if xpos > 8:
                    wrongSettings += 1

We use pass only in a situation where it is syntactically necessary, for example when defining a new Error class that doesn't override any of the behaviors it inherits.

simplify logic

Validating coordinates works fine as-is. But rather than looping, it would be simpler and more direct to just unpack the expected pair of characters and validate each one:

        assert isinstance(k, str)
        file, rank = k  # or str(k), if I guessed wrong and k isn't always a string
        if file not in valid_fields: ...
        if rank not in range(1, 9): ...

When creating the black_ and white_chess dicts, you similarly should not need str(k). Just use if k.startswith("w"):

default dict

        valueCheck.setdefault(v, 0)
        valueCheck[v] = valueCheck[v] +1

Ok, now I'm starting to understand this data structure. It appears it should have been named checked_values.

Initialize in this way:

from collections import defaultdict
...
    checked_values = defaultdict(int)
...
        checked_values[v] += 1

@rdesparbes observes that Counter offers similar functionality.

consistent error reporting

        print("Too many pieces")

Recommend you report "Too many white pieces". And then fill in the corresponding black report, which currently is missing.

This function is getting to be Too Long, and counting pieces is one of several excellent opportunities to break out a small helper function.

API design

This is kind of weird:

    wrongSettings = figureChecks(whiteChess, wrongSettings)

It would be more natural to phrase it:

    wrong_settings += figure_checks(white_chess)

No need for figure_checks() to know about how many errors have happened already, since that won't alter its result.

use a boolean expression to return a boolean

    if wrongSettings > 0:
        return False
    else:
        return True

Prefer:

    return wrongSettings == 0
\$\endgroup\$
2
  • 2
    \$\begingroup\$ FYI - brett is German for 'board' (Schachbrett == chessboard) Looks like it just might have been one of the few things OP didn't translate to English before posting. \$\endgroup\$
    – RisingZan
    Jan 17 at 7:10
  • 1
    \$\begingroup\$ valueCheck could also be a collections.Counter \$\endgroup\$
    – rdesparbes
    Jan 18 at 14:16
3
\$\begingroup\$

Your function chessCheck is a good start to validating a chessboard, but there are several improvements and additions that could be made to ensure it thoroughly checks for all the criteria of a valid chessboard. Here are some suggestions:

Handling Piece Names and Counts: You've started a valueCheck dictionary but haven't used it yet. This dictionary should be used to count the occurrences of each piece. Ensure that each player has no more than 16 pieces in total and no more than 8 pawns.

Checking Piece Validity: The function should verify that each piece is one of the valid types ('pawn', 'knight', 'bishop', 'rook', 'queen', 'king') and is prefixed correctly with either 'w' for white or 'b' for black.

def isValidChessBoard(board):
         validFields = [str(i) + c for i in range(1, 9) for c in "abcdefgh"]
         validPieces = ['pawn', 'knight', 'bishop', 'rook', 'queen', 'king']

Validating Board Positions: Instead of checking each character in the key string, you can split the key into the numeric and alphabetic parts. The numeric part should be in the range 1 to 8, and the alphabetic part should be in the list validFields.

Counting Kings: Check for the existence of exactly one black king and one white king.

Error Handling: Add error handling for cases where the input is not as expected (e.g., if the keys are not strings or if the dictionary contains unexpected values).

\$\endgroup\$
3
\$\begingroup\$

Write tests

Your code has bugs and inconsistent behavior. Designing proper test coverage would likely have caught these issues, and can help you figuring out what the code actually has to do.

Consider the following boards:

valid_1 = {'1a' : 'wking',
           '1b' : 'wqueen',
           '1c' : 'wqueen',
           '8d' : 'bking',
           '8e' : 'bqueen'}
valid_2 = {'1a' : 'wking',
           '1b' : 'wqueen',
           '8d' : 'bking',
           '8e' : 'bqueen',
           '8f' : 'bqueen'}

By the exercise's specification, both of these board are valid:

  • exactly one white and one black king
  • less than 16 pieces for each player
  • less than eight pawns
  • all pieces are in valid spaces
  • all pieces are correctly named

(In fact, they are also valid boards according to chess rules, which are a lot more complex to validate: a pawn can be promoted to a queen, so there can be more than 1 queen.)

Yet, your code rejects the first one as having too many white queens, but accepts the second one, that has 2 black queens.

Inversely, the following board should be rejected, as it contains an invalid piece name, but is considered valid by your code:

invalid = {'1a' : 'wking',
           '1b' : 'wfoo',
           '8d' : 'bking'}

Ideally, you want to use a unit testing framework, such as the unittest module, which is part of the Python standard library. Try to come up with tests that verify each part of the specifications, with both valid and invalid tests. This would look something like:

import unittest

from chessboard_validator import is_valid_board


class ChessboardValidatorTest(unittest.TestCase):

    def test_valid_board(self):
        # example in the exercise
        self.assertTrue(is_valid_board(
            {'1h': 'bking', 
             '6c': 'wqueen', 
             '2g': 'bbishop', 
             '5h': 'bqueen', 
             '3e': 'wking'}
            ))
        # TODO: add tests for other boards, including:
        #      - starting position
        #      - many queens or other pieces
        
        
    def test_not_enough_kings(self):
        self.assertFalse(is_valid_board(
            {'1h': 'bking'}
            ))
        self.assertFalse(is_valid_board(
            {'1h': 'wking'}
            ))
        # TODO: maybe add tests for more complex boards missing a king.
        
    def test_too_many_kings(self):
        # TODO
        pass
    
    def test_too_many_pieces(self):
        # TODO
        pass
    
    def test_too_many_pawns(self):
        # TODO
        pass
    def test_piece_in_invalid_space(self):
        # TODO
        pass
    
    def test_invalid_piece_name(self):
        # TODO
        pass
        


if __name__ == '__main__':
    unittest.main()

Now that you have proper test coverage, you can be verify whether your code is correct or not, and if any change you implement improve or break your code.

Separation of concerns

The exercise specifications explicitly require a boolean value to be returned by your code. Your code does that, but it also mixes in other types of output by (sometimes) printing additional information.

You will see why this is a problem if you run the tests: your test results will be much harder to read if they are spammed with print statements from your code.

It's also implicit that such a function will be a part of a larger code base, probably a chess game implementation. If it's a terminal app, the print statements will disrupt the CLI display. If it's a GUI app, where is it supposed to print?

If you really want additional debug output, consider using a proper logging utility.

Naming

Your naming doesn't follow PEP8's naming conventions and are often meaningless single-letter names. This makes it hard to follow along the code.

Going further

If you follow these suggestion, and advice given in other answers, you might want to implement other validation rules, to get closer to (or fully implement) actual chess rules. Figure out what the actual requirement is, write new tests, and implement them in code.

It can be a good exercise, as modifying existing code is a different skill than starting from a blank slate, and is much more common in practice.

Rules that you can consider implementing:

  • There can't be more than one piece on any tile
  • White pawns can't be on row 1, black pawns can't be on row 8
  • There can be at most 2 rooks, knights and bishops, and 1 queen, unless pawns have been promoted. There can be at most 8 promotion for each player.
  • Bishops must be on tiles of different color, unless at least one pawn has been promoted.
\$\endgroup\$

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service and acknowledge you have read our privacy policy.

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