I am working on my Python skills and would like some feedback on this problem I have tried to solve. I would like to return a boolean if two knights at their current position can attack each other using standard chess notation example (4,'A')

from itertools import product

class ChessPosition:
    def __init__(self, number, letter):
        self.x = number
        self.y = letter
        self.notation_file_value = {
            "A": 1,
            "B": 2,
            "C": 3,
            "D": 4,
            "E": 5,
            "F": 6,
            "G": 7,
            "H": 8,

    def get(self):
        return self.x, self.notation_file_value.get(self.y)

# get all possible knight moves from current position

def knight_moves(position):
    x, y = position
    moves = list(product([x - 1, x + 1], [y - 2, y + 2])) + list(
        product([x - 2, x + 2], [y - 1, y + 1]))
    moves = [(x, y) for x, y in moves if x >= 1 and y >= 1 and x < 8 and y < 8]
    return moves

# validate if knights can attack each other from current position
# if chess_position_1 can attack chess_position_2 
# then chess_position_2 can attack chess_position_1

def can_attack(chess_position_1, chess_position_2):
    k_1 = chess_position_1.get()
    k_2 = chess_position_2.get()

    for move in knight_moves(k_1):
        if move == k_2:
            return True
    return False

# True
print(can_attack(ChessPosition(2, "C"), ChessPosition(4, "D")))

# False
print(can_attack(ChessPosition(6, "A"), ChessPosition(5, "B")))

1 Answer 1


It's kind of a stretch to apply OOP here, but fine. Keeping it, you should type-hint your number and letter arguments. Consider re-phrasing your notation_file_value as a subtraction of two character ordinals.

Your algorithm can be greatly simplified. Don't generate all moves - instead, just subtract the destination and source being checked, and if they're either (2, 1) or (1, 2) in an absolute sense, that's a valid knight move. Also, it's better named a move than an attack, since this logic will apply to non-attack moves.

Convert your prints to simple unit tests.


class ChessPosition:
    def __init__(self, x: int, y: int) -> None:
        self.x, self.y = x, y

    def from_grid(cls, row: int, column: str) -> 'ChessPosition':
        return cls(row - 1, ord(column) - ord('A'))

    def __sub__(self, other: 'ChessPosition') -> tuple[int, int]:
        return self.x - other.x, self.y - other.y

def knight_can_reach(source: ChessPosition, dest: ChessPosition) -> bool:
    dx, dy = dest - source
    return sorted((abs(dx), abs(dy))) == [1, 2]

def test() -> None:
    assert knight_can_reach(ChessPosition.from_grid(2, 'C'), ChessPosition.from_grid(4, 'D'))
    assert not knight_can_reach(ChessPosition.from_grid(6, 'A'), ChessPosition.from_grid(5, 'B'))

if __name__ == '__main__':
  • \$\begingroup\$ How would from_grid handle a negative value passed for row? \$\endgroup\$
    – robotjeans
    May 9, 2022 at 18:01
  • 1
    \$\begingroup\$ @ShawnSheehan It does no validation of the position, so it will silently accept it. If you want to validate the position, you could do so within __init__. \$\endgroup\$
    – Reinderien
    May 9, 2022 at 18:17
  • 2
    \$\begingroup\$ FYI, the "file" in notation_file_value refers to the files (columns) of a chessboard, not to a filesystem entry. \$\endgroup\$
    – jwodder
    May 9, 2022 at 19:42
  • \$\begingroup\$ yes @jwodder that's what I was going for but I can see how the name could be confusing. \$\endgroup\$
    – robotjeans
    May 9, 2022 at 20:08

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.