Skip to main content
added 32 characters in body
Source Link
SuperBiasedMan
  • 13.1k
  • 5
  • 36
  • 62
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:
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
squares = zip(range(y, -1, -1), range(x, -1, -1)) 
for x1, y1 in squares:

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, 8), range(x, 8)),
                zip(range(y, 8), range(x, -1, -1)),
                zip(range(y, -1, -1), range(x, 8)),
                zip(range(y, -1, -1), range(x, -1, -1)),
             ]
for direction in directions:
    for square in direction:
def isvalid_bishop_move(start):
    '''Return a list of all legal moves for bishop on square'''
    
    squares = []
    y, x = square

    directions = [
                    zip(range(y, 8), range(x, 8)),
                    zip(range(y, 8), range(x, -1, -1)),
                    zip(range(y, -1, -1), range(x, 8)),
                    zip(range(y, -1, -1), range(x, -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
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:
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
Source Link
SuperBiasedMan
  • 13.1k
  • 5
  • 36
  • 62

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), range(x, -1, -1)) 
for x1, y1 in squares:

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, 8), range(x, 8)),
                zip(range(y, 8), range(x, -1, -1)),
                zip(range(y, -1, -1), range(x, 8)),
                zip(range(y, -1, -1), range(x, -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, 8), range(x, 8)),
                    zip(range(y, 8), range(x, -1, -1)),
                    zip(range(y, -1, -1), range(x, 8)),
                    zip(range(y, -1, -1), range(x, -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