Finding all legal bishop moves

Question

I am trying to create a two player Chess game. To do this I want to generate a list of legal moves. I am having trouble with the brevity of the code to generate the moves for the rooks and bishops.

The code I have pasted below works (i.e. shows legal bishop moves) but there is a lot of repetition, which is not ideal. To make this work for both black and white bishops I would need to add even more repetitive lines.

Is there a better way to do this? I do not want to use any 'chess modules' as this is a learning exercise for me; as you can tell, I am a beginner.

import numpy as np

# chess board is a numpy array
# white pieces are capitalised, black pieces are not
board = np.zeros((8,8),dtype=str)

# example piece positions
white_bishop = (4,5)
white_pawn = (6,7)
black_knight = (0,1)
board[white_bishop] = 'B'
board[white_pawn] = 'P'
board[black_knight] = 'n'

def bishop(insquare):
    '''
    Gets all legal moves for bishop on insquare
    output is a list (outsquares) of indices of possible squares
    '''
    out_squares = []
    y,x = insquare

    y1 = y-1 # these are steps in one of the four directions bishops can go
    x1 = x-1
    while (x1 > -1) and (y1 > -1): # edge of the board
        if not board[(y1,x1)].isupper(): # if not a white piece...
            out_squares.append((y1,x1)) # square is legal
            if board[(y1,x1)].islower(): # if black piece...
                y1 = -1 # look no further in this direction
        else: # if white piece...
            y1 = -1 # look no further in this direction
        y1 -= 1 # check next square
        x1 -= 1

    # check all other directions
    y1 = y+1
    x1 = x+1
    while (x1 < 8) and (y1 < 8):
        if not board[(y1,x1)].isupper():
            out_squares.append((y1,x1))
            if board[(y1,x1)].islower():
                y1 = 8
        else:
            y1 = 8
        y1 += 1
        x1 += 1

    y1 = y + 1
    x1 = x - 1
    while (x1 > -1) and (y1 < 8):
        if not board[(y1,x1)].isupper():
            out_squares.append((y1,x1))
            if board[(y1,x1)].islower():
                y1 = 8
        else:
            y1 = 8
        y1 += 1
        x1 -= 1

    y1 = y - 1
    x1 = x + 1
    while (x1 < 8) and (y1 > -1):
        if not board[(y1,x1)].isupper():
            out_squares.append((y1,x1))
            if board[(y1,x1)].islower():
                y1 = -1
        else:
            y1 = -1
        y1 = y1 - 1
        x1 = x1 + 1
    return out_squares

# Get all legal moves for the white bishop
print bishop((4,5))
# >>> [(3, 4), (2, 3), (1, 2), (5, 6), (5, 4), (6, 3), (7, 2), (3, 6), (2, 7)]

Answer 1

When you find yourself having a lot of duplicate code, you should factor out the commonalities. In this case, we have four separate directions we need to consider. So let's write one function that considers one direction:

def append(dx, dy):
    for i in itertools.count(start=1):
        newx = x + dx*i
        newy = y + dy*i
        sq = (newy, newx)

        if 0 <= newx < 8 and 0 <= newy < 8:
            if not board[sq].isupper():
                out_squares.append(sq)
                if board[sq].islower():
                    break
            else:
                break
        else:
            break

And then just call it 4 times:

for dx in (-1, 1):
    for dy in (-1, 1):
        append(dx, dy)

Full solution:

def bishop(insquare):
    ''' 
    Gets all legal moves for bishop on insquare
    output is a list (outsquares) of indices of possible squares
    '''
    out_squares = []
    y,x = insquare

    def append(dx, dy):
        for i in itertools.count(start=1):
            newx = x + dx*i
            newy = y + dy*i

            if 0 <= newx < 8 and 0 <= newy < 8:
                sq = (newy, newx)
                if not board[sq].isupper():
                    out_squares.append(sq)
                    if board[sq].islower():
                        break
                else:
                    break
            else:
                break

    for dx in (-1, 1):
        for dy in (-1, 1):
            append(dx, dy) 

    return out_squares

Answer 2

Starting from the start, you should use different names for both your function and parameter. bishop sounds like a classname, isvalid_bishop_move or something similar is more suitable. Your parameter on the other hand is needlessly made more awkward by adding in. square would be fine, or location. insquare implies something more specific or complicated than just a coordinate. It seems like you're using this as a naming convention to mean the current square a piece is on and the places it can move to? You don't explain it and it doesn't seem necessary to me either.

Your docstring is also split into two when you should make it one coherent line.

'''Return a list of all legal moves for bishop on square'''

Now, the main thing is that you've mistakenly used while loops. If you made these range objects instead you could iterate over all of them and remove your looping. range(a, b) will create a list or generator (depending on your Python version) from a to b, exclusing b. So your first while loop could just refer to the x and y co-ordinates from x to -1 and y to -1. So your ranges would be:

range(x, -1, -1)
range(y, -1, -1)

The third parameter, -1 will make the loop iterate from a number down. ie. range(3, -1, -1) => [3, 2, 1, 0]. Of course you want to loop over both at once, so you can attach the two of them together using zip. It will take two lists and combine them into one list with a set of tuples. For example:

zip(range(5, -1, -1), range(3, -1, -1))
>>> [(5, 3), (4, 2), (3, 1), (2, 0)]

Notice that it stops when either one of the lists runs out of items, this is perfect for your case. Now you can iterate over the result of this zip instead of the while.

squares = zip(range(y - 1, -1, -1), range(x - 1, -1, -1)) 
for x1, y1 in squares:

(note we start one square on from x and y because you don't want to include the current square)

Now, why does this help? Because you could just build all 4 sets of squares this way and put them in one list of directions. Then you can iterate over those, like this:

directions = [
                zip(range(y + 1, 8), range(x + 1, 8)),
                zip(range(y + 1, 8), range(x - 1, -1, -1)),
                zip(range(y - 1, -1, -1), range(x + 1, 8)),
                zip(range(y - 1, -1, -1), range(x - 1, -1, -1)),
             ]
for direction in directions:
    for square in direction:

Now you only need to have one block of code to do each direction. Technically this breaks your y1 = -1 condition, but that's a bad approach anyway. You can use break to immediately stop a loop whether it's a while loop or a for loop and that's easier to follow for another user reading your code.

if not board[(y1,x1)].isupper():
    out_squares.append((y1,x1))
    if board[(y1,x1)].islower():
        break
else:
    break

It's also clearer if you first check if the string is upper and then break, otherwise do the next test. You can write more understandable comments that way:

# Is it a white piece?
if board[(y1,x1)].isupper():
    break

out_squares.append((y1,x1))
# Is it a black piece?
if board[(y1,x1)].islower():
    break

So here's how the full function would look:

def isvalid_bishop_move(start):
    '''Return a list of all legal moves for bishop on square'''

    squares = []
    y, x = square

    directions = [
                    zip(range(y + 1, 8), range(x + 1, 8)),
                    zip(range(y + 1, 8), range(x - 1, -1, -1)),
                    zip(range(y - 1, -1, -1), range(x + 1, 8)),
                    zip(range(y - 1, -1, -1), range(x - 1, -1, -1)),
                 ]

    for direction in directions:
        for square in direction:
            # Is it a white piece?
            if board[(y1,x1)].isupper():
                break

            squares.append((y1,x1))
            # Is it a black piece?
            if board[(y1,x1)].islower():
                break

    return squares