Nested conditionals vs conditionals connected by operators

[dboxall123]

The following code is a short extract from a game I'm making, called tablut, also known as viking chess.

def check_for_kills(self, player, row, col):
		typ = self.pieces[row][col].type
		deaths = []
		# check for pieces to the left
		if col >= 2: # prevents IndexError
			if self.pieces[row][col-1] is not None: 
				if self.pieces[row][col-2] is not None:
					if self.pieces[row][col-1].type != typ:
						if self.pieces[row][col-2].type == typ:
							deaths.append((row, col-1))
        return deaths

A similar set of if statements is repeated 3 more times, one for each direction (right, down, up). Initially, I had done this:

def check_for_kills(self, player, row, col):
		typ = self.pieces[row][col].type
		deaths = []
		# check for pieces to the left
		if col >= 2: # prevents IndexError
			if self.pieces[row][col-1] is not None and self.pieces[row][col-2] is not None:
					if self.pieces[row][col-1].type != typ and self.pieces[row][col-2].type == typ:
							deaths.append((row, col-1))
        return deaths

I had also thought about doing something like this:

def check_for_kills(self, player, row, col):
		typ = self.pieces[row][col].type
		deaths = []
		# check for pieces to the left
		if col >= 2: # prevents IndexError
			if self.pieces[row][col-1] is not None and \
				self.pieces[row][col-2] is not None and \
					self.pieces[row][col-1].type != typ and \
						self.pieces[row][col-2].type == typ:
							deaths.append((row, col-1))
        return deaths

Which is preferable, and why? I chose to use the first example, because I believe it to be more readable. Are there performance differences between using a number of nested conditionals and using a single if statement with a load of conditionals connected by and operators? Thanks

[Gribouillis]

I suggest this more readable version

def check_for_kill(self, player, row, col)
    r = self.pieces[row]
    if col >= 2:
        c, c1, c2 = r[col], r[col - 1], r[col - 2]
        if c1 and c2 and c1.type != c.type and c2.type == c.type:
            deaths.append((row, col-1))
    return deaths

I don't think there will be a significant performance difference between the versions. The way to answer this is by measuring performance with the timeit module.

[deanhystad]

Being Truey is not a good test for not None unless the only possible values are None and something that is always Truey. This also doesn't check if c is not None. That check is also left out of dboxall's code. Maybe that condition is checked external to the function.

if c1 and c2 and c1.type != c.type and c2.type == c.type:

I like slicing.

class Thing:
    def __init__(self, value):
        self.value = value
        self.type = type(value)

class X:
    def __init__(self, pieces):
        pieces = [None if value is None else Thing(value) for value in pieces]
        self.pieces = [pieces]

    def check_for_kill(self, row, col):
        if col > 1:
            a, b, c = self.pieces[row][col-2:col+1]
            if not None in (a, b, c) and a.type == c.type and b.type != c.type:
                return[(row, col-1)]
        return []

    # Or this

    def check_for_kill(self, row, col):
        if col > 1:
            c = self.pieces[row][col-2:col+1]
            if not None in c and c[0].type == c[2].type and c[1].type != c[2].type:
                return[(row, col-1)]
        return []

print(X([0, 0.0, 1]).check_for_kill(0, 2))
print(X([0, 0.0, 1]).check_for_kill(0, 1))
print(X([0, 0, 1]).check_for_kill(0, 2))
print(X([None, 0.0, 1]).check_for_kill(0, 2))

Output:
[(0, 1)]
[]
[]
[]

[dboxall123]

I suggest this more readable version

def check_for_kill(self, player, row, col)
    r = self.pieces[row]
    if col >= 2:
        c, c1, c2 = r[col], r[col - 1], r[col - 2]
        if c1 and c2 and c1.type != c.type and c2.type == c.type:
            deaths.append((row, col-1))
    return deaths

I don't think there will be a significant performance difference between the versions. The way to answer this is by measuring performance with the timeit module.

Yeah, that looks better. I was wondering about using if var: vs if var is not None:, because I've always just used the former, but have seen the latter in code examples onliine many times. I looked it up online, and found many on stackoverflow advising against it. One posted the follwing extract from PEP:

Comparisons to singletons like None should always be done with 'is' or 'is not', never the equality operators.

Also, beware of writing "if x" when you really mean "if x is not None" -- e.g. when testing whether a variable or argument that defaults to None was set to some other value. The other value might have a type (such as a container) that could be false in a boolean context!

But I suppose that if I know for certain that what I'm testing can only be None or Piece(), and definitely cannot accidentally evaluate to False, then it's acceptable, right? Anyway, thanks!

[Gribouillis]

If you know that the value can be only None or a Piece, checking truth instead of None value is perfectly OK.
Instead of returning a list, you could also generate the squares to kill. Finally, you could handle all the directions with code like

def spam(self, row, col):
    if row - 2 >= 0:
        yield ((row-1, col), (row-2, col))
    if row + 2 < self.maxrows:
        yield ((row+1, col), (row+2, col))
    if col - 2 >= 0:
        yield ((row, col-1), (row, col-2))
    if col + 2 < self.maxcols:
        yield ((row, col+1), (row, col+2))

def check_for_kill(self, row, col):
    p = self.pieces

    x = p[row][col]
    for (r, c), (rr, cc) in self.spam(row, col):
        y, z = p[r][c], p[rr][cc]
        if y and z and y.type != x.type and z.type == x.type:
            yield (r, c)

[dboxall123]

That's great, thanks. I must admit, I've never actually used yield. I remember trying to learn it a few years ago, when I was new to Python, and programming in general, and it just went over my head, I couldn't understand it. Then I forgot about it until today lol. Interestingly, having just read a quick tutorial online, I honestly can't see what was so difficult about it back then!
I also didn't know that this could be done:

lst = [(1,2), (3,4)]
for x, y in lst:
    #do stuff with x
    #do stuff with y

I've always just used for x in lst: and then #do stuff with x[index]so thanks for that, that will definitely come in handy!

[dboxall123]

Being Truey is not a good test for not None unless the only possible values are None and something that is always Truey. This also doesn't check if c is not None. That check is also left out of dboxall's code. Maybe that condition is checked external to the function.

if c1 and c2 and c1.type != c.type and c2.type == c.type:

I like slicing.

class Thing:
    def __init__(self, value):
        self.value = value
        self.type = type(value)

class X:
    def __init__(self, pieces):
        pieces = [None if value is None else Thing(value) for value in pieces]
        self.pieces = [pieces]

    def check_for_kill(self, row, col):
        if col > 1:
            a, b, c = self.pieces[row][col-2:col+1]
            if not None in (a, b, c) and a.type == c.type and b.type != c.type:
                return[(row, col-1)]
        return []

    # Or this

    def check_for_kill(self, row, col):
        if col > 1:
            c = self.pieces[row][col-2:col+1]
            if not None in c and c[0].type == c[2].type and c[1].type != c[2].type:
                return[(row, col-1)]
        return []

print(X([0, 0.0, 1]).check_for_kill(0, 2))
print(X([0, 0.0, 1]).check_for_kill(0, 1))
print(X([0, 0, 1]).check_for_kill(0, 2))
print(X([None, 0.0, 1]).check_for_kill(0, 2))

Output:
[(0, 1)]
[]
[]
[]

Apologies, I didn't notice your reply, it is above all the others. Firstly, yes, I know for certain that in the list I'm checking there is only None and Piece objects, there is nothing else that can evaluate to False. Secondly, yes, the "c" or "col" variable has already been verified externally (it has to evaluate to true, because (row, col) is the position to which a piece has just been moved to). Thanks for your help!

[deanhystad]

Worrying about the performance of an if statement inside a function is a bit kooky. If you want speed, you get more efficiency by not calling a function. Gribouilis' generator or a function that processes a row or the entire board at a time will save a lot more time than the operators used in your condition statement.

This code does a the entire board (Please excuse my ignorance of Viking Chess if it does not). Not only would this be a lot faster, it is even easier to read.

def check_for_kills(self):
    kills = []
    for row in range(len(self.pieces)):
        a = b = None
        for col, c in enumerate(self.pieces[row]):
            if all(a, b, c):
                if a.type == c.type and b.type != c.type:
                    kills.append((row, col-1))
            a, b = b, c
    return kills

[dboxall123]

Worrying about the performance of an if statement inside a function is a bit kooky. If you want speed, you get more efficiency by not calling a function. Gribouilis' generator or a function that processes a row or the entire board at a time will save a lot more time than the operators used in your condition statement.

This code does a the entire board (Please excuse my ignorance of Viking Chess if it does not). Not only would this be a lot faster, it is even easier to read.

def check_for_kills(self):
    kills = []
    for row in range(len(self.pieces)):
        a = b = None
        for col, c in enumerate(self.pieces[row]):
            if all(a, b, c):
                if a.type == c.type and b.type != c.type:
                    kills.append((row, col-1))
            a, b = b, c
    return kills

I wouldn't go so far as to say that I'm worrying about it, just curious really. As for checking the entire board, I had initially entertained the idea, but I decided not to go through with it, because of one of the rules of the game. A piece is "killed" if it is surrounded either vertically or horizontally by the opposing side. However, if you move a piece between two pieces of teh enemy, then you're considered safe. That's why I decided to check for kills based purely on where a piece has just moved to. Anyway, thanks for the help everyone.