Simon said:
Hey I was hoping to get your opinions on a sort of minor stylistic
point.
These two snippets of code are functionally identical. Which would you
use and why?
The first one is easier [for me anyway] to read and understand, but
slightly less efficient, while the second is [marginally] harder to
follow but more efficient.
if self.higher is self.lower is None: return
if self.lower is None: return self.higher
if self.higher is None: return self.lower
if self.higher is None:
if self.lower is None:
return
return self.lower
if self.lower is None:
return self.higher
Explicit is always better, `return None` when that is your intent.
`return` shouts to the reader, "I want to get out of this function now,
and no one cares about the return result."
Personally, I only use the one-liner if/else clauses if it's an
extremely trivial condition, and even then, usually not there. (The
only place I use it consistently is `if __name__ == '__main__':
main()`.) If it's part of something more important that needs
inspection -- as this obviously does given the questions you've had
about it, doesn't skip space. Newlines are free.
Even when expanding about the first test, there's no reason to do
chained `if`s. One `if` with an `and` works just fine.
Paul Rubin's looked to be the best overall replacement, as the logic
here looks strangely turned inside out. You're obviously looking for
which one _isn't_ `None`, so write the tests that way. It's much easier
for everyone else (including your potential future self) to follow.
Thanks for all the great feedback!
Some notes:
This code is part of a delete() method for a binary tree
implementation. "None" is used to simulate a NULL pointer. In the case
where both children are non-None the code goes on to handle that.
None is a "unitary" or "singleton" value, so "is" is the right
comparison operator to use with it, at least in this "imitation
pointer" usage.
The bare return was used both to preclude executing the rest of the
function and to return None. (I sometimes use "return # None" with
None in a comment to note that the return value is used. Also, I will
put "# return" or "# return None" at the end of a function if the
returning-of-None is important to the implementation of whatever.
I've never used dis.dis() to check the bytecodes, but I wouldn't be
surprised to find that the compiler generated the same bytecode
whether you explicitly state the return or comment it out.)
In any event, as Duncan Booth pointed out, the entire line was
redundant so the issue is somewhat moot in this particular case.
As for one-line control flow statements in python, I usually refrain
but this code wasn't originally meant for public viewing.
Last but not least, the logic may seem backwards but it's actually
correct. If you check for non-None (NULL) -ness and return the thing
you checked, rather than checking for None-ness and returning the
other, the case where both are non-None is not handled correctly.
Thanks again,
~Simon
FWIW, here's the BinaryTree class, the code is in the _delete_me()
method.
from random import random
class BinaryTree:
'''
A Binary Tree implementation. Provides:
get(key) - Return item for key or None.
insert(key, value) - Insert value under key, replacing old
ones.
delete(key) - Delete value under key if any.
keys() - Iterate through the keys in sort order.
values() - Iterate through the values in sort order.
items() - Iterate through (key, value) pairs in sort order.
'''
def __init__(self, key, value):
self.key = key
self.value = value
self.higher = self.lower = None
def get(self, key):
'''
Return item for key or None.
'''
if key == self.key:
return self.value
if key < self.key and self.lower is not None:
return self.lower.get(key)
if self.higher is not None:
assert key > self.key
return self.higher.get(key)
def insert(self, key, value):
'''
Insert value under key, replacing old ones.
'''
if key == self.key:
self.value = value
elif key < self.key:
if self.lower is None:
self.lower = BinaryTree(key, value)
else:
self.lower.insert(key, value)
else:
assert key > self.key
if self.higher is None:
self.higher = BinaryTree(key, value)
else:
self.higher.insert(key, value)
def delete(self, key):
'''
Delete value under key if any.
'''
if key < self.key and self.lower is not None:
self.lower = self.lower.delete(key)
elif key > self.key and self.higher is not None:
self.higher = self.higher.delete(key)
elif key == self.key:
return self._delete_me()
return self
def _delete_me(self):
if self.lower is None: return self.higher
if self.higher is None: return self.lower
# Two children...
try:
side = self._which_side
except AttributeError:
side = bool(int(random() * 2))
# Alternate sides, should help keep tree balanced.
self._which_side = not side
method = self._delete_lower if side else self._delete_higher
return method()
def _delete_lower(self):
next = self.lower
while next.higher is not None: next = next.higher
self.key = next.key
self.value = next.value
self.lower = self.lower.delete(self.key)
return self
def _delete_higher(self):
next = self.higher
while next.lower is not None: next = next.lower
self.key = next.key
self.value = next.value
self.higher = self.higher.delete(self.key)
return self
def keys(self):
'''
Iterate through the keys in sort order.
'''
for key, value in self.items():
yield key
def values(self):
'''
Iterate through the values in sort order.
'''
for key, value in self.items():
yield value
def items(self):
'''
Iterate through (key, value) pairs in sort order.
'''
if self.lower is not None:
for kv in self.lower.items():
yield kv
yield self.key, self.value
if self.higher is not None:
for kv in self.higher.items():
yield kv
