question of style

[Simon Forman]

On Jul 2, 4:08 pm, Erik Max Francis <m... at alcyone.com> wrote:
> Simon Forman wrote:
> > 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.
>
> > ## First snippet
>
> > 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
>
> > ## Second snippet
>
> > if self.higher is None:
> >     if self.lower is None:
> >         return
> >     return self.lower
> > if self.lower is None:
> >     return self.higher
>
> > What do you think?
>
> 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