I started a Chess in Python for fun. I learned Python in my free time but I've been doing it for a while. The code works but I just want to know if there's anything non-pythonic in my code. The functions calculate all potentially possible moves (ignoring game state).

from itertools import product
from itertools import chain
from functools import wraps
from math import atan2

_angle_to_direction = {90: "up", -90: "down", 180: "right", 0: "left", -135: "right_down", 135: "right_up",
                       45: "left_up", -45: "left_down"}

def move(f):
    def wrapper(*args, **kwargs):
        x, y = args
        moves = f(x, y)
        if (x, y) in moves: moves.remove((x, y))
        return moves

    return wrapper

def check_range(x:tuple) -> bool:
    return x[0] >= 0 and x[1] >= 0 and x[0] < 8 and x[1] < 8

def _knight(x:int, y:int) -> set:
    moves = chain(product([x - 1, x + 1], [y - 2, y + 2]), product([x - 2, x + 2], [y - 1, y + 1]))
    moves = {(x, y) for x, y in moves if check_range((x, y))}
    return moves

def _rook(x:int, y:int) -> set:
    return {(x, i) for i in range(0, 8)}.union({(i, y) for i in range(0, 8)})

def _bishop(x:int, y:int) -> set:
    possible = lambda k: [(x + k, y + k), (x + k, y - k), (x - k, y + k), (x - k, y - k)]
    return {j for i in range(1, 8) for j in possible(i) if check_range(j)}

def direction(start:tuple, end:tuple) -> str:
    delta_a = start[1] - end[1]
    delta_b = start[0] - end[0]
    return _angle_to_direction[int(atan2(delta_a, delta_b) * 180 / 3.14)]

The implementation of your move decorator is a little schizophrenic. On the one hand, it carefully allows any and all arguments to be passed to it by declaring itself def wrapper(*args, **kwargs). However it then requires exactly two positional arguments which it then passes on to the wrapped function. Keyword arguments are accepted but ignored and dropped. I would suggest changing this to be all at one end or all at the other end of this spectrum. Given the annotations elsewhere, I would expect you to prefer changing the function to def wrapper(x, y) (or maybe to def wrapper(x, y, **kwargs) but then also passing **kwargs to f).

It might make sense to incorporate check_range into the wrapper as well, simplifying most of the move generators. It might also make sense to skip the decorator and instead call the filter in your move generators: return filter_move((x, y), moves).

As a side note, it would be interesting to see if there are any performance advantages to either the current approach of checking for (x,y) and conditionally removing it or to unconditionally doing a set difference: return f(x, y) - {(x, y)}.

The rest of your code is fairly straightforward, if perhaps a bit too clever. Clever is typically bad for other readers, or for debugging misbehavior. The two things that stand out to me is the name of your lambda possible (which seems more like a diagonal_moves_of_distance or simply diagonal), and the approach used for direction. Given the limited number of possible move deltas, bringing in trigonometry to handle it seems like overkill; I'd almost expect a simple if/else tree instead. As is, it will have problems classifying a knight's move.

  • \$\begingroup\$ thank you sir. i would agree with you being too clever is bad that was my main concern. very detailed answer much appreciated \$\endgroup\$ – Foo Bar User Dec 11 '13 at 19:01

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service, privacy policy and cookie policy

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