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