[OH-Dev] Code style question: lines of code vs. indenting complexity

[will kahn-greene]

Way 5 could be adjusted with a lambda which would only execute 
_calculate_value() iff the key isn't in the cache:

<WAY_5>
return cache.get(key, lambda: _calculate_value())
</WAY_5>

That still doesn't put the key in the cache, but this sort of "don't 
execute me unless you must" sort of thing is a common use for lambda.

I'm a fan of WAY_1 since the code returns on the obvious conditions 
immediately and then you get to the meat of the issue without having to 
maintain the quantum baggage.

Regardless, there's a WAY_6 that I tend to prefer.  Namely, create a 
caching decorator and then decorate the function/method in question.  
Then your thing turns into:

<WAY_6>
@cachify
def some_func():
    return _calculate_value()
</WAY_6>

The thing I like about this is that caching is usually a side-effect 
and it's nice for reasoning purposes to keep it separate from the code 
that's doing things.

/will


On Thu 03 Nov 2011 08:58:33 PM EDT, Asheesh Laroia wrote:
> (If you don't care about the minutiae of code style discussions, feel
> free to delete/archive/mute this thread.)
>
> In 4ebb98836e5a01389b64362165a4f0e663bd1e96 in the "master" branch on
> oh-mainline, Jack changed the style of some code of mine (while
> retaining the logic as the same). That's generally fine, and in this
> specific case fine.
>
> If you do "git show" on that revision, you will see a style
> disagreement. I wanted to discuss a style question that this speaks to:
>
> Which is better -- more indenting, or a longer function?
>
> The _get_importer_instance_for_tracker_model() method has this general
> structure:
>
> * From the inputs to the method, generate a key.
> * Look up that key in a cache.
> * If the cache is empty for that key, generate a value.
> * Return the value, either from cache or generated.
> * (Make sure that the value gets stored in the cache.)
>
> There are approximately two ways to do the cache-fill part of this:
>
> <WAY_1>
>
> if key in cache:
>    return cache[key]
>
> value = # calculate value...
> cache[key] = value
> return cache[key]
>
> </WAY_1>
>
> <WAY_2>
>
> if key not in cache:
>     value = # calculate value...
>     cache[key] = value
> return cache[key]
>
> </WAY_2>
>
> WAY_2 is shorter than WAY_1 by two lines, but I prefer WAY_1 for the
> following reasons:
>
> First, I don't like negated "if" conditions. They're harder for me to
> read than positive tests.
>
> Second, the value-calculation work is indented one extra level. The
> more indented code becomes, the shorter its lines have to be in order
> to fit within people's editors, and also generally the more complex it
> feels.
>
> I suppose I can mitigate the second issue by moving the
> value-calculation code into a new helper method.
>
> There's a third possible way to do it:
>
> <WAY_3>
> if key not in cache:
>     value = # calculate value...
>     cache[key] = value
>     # Now that the cache is full, do a recursive call
>     return _get_importer_instance_for_tracker_model()
>
> return cache[key]
> </WAY_3>
>
> There's a fourth way, but it is wrong:
>
> <WAY_4>
> return cache.get(key, _calculate_value())
> </WAY_4>
>
> It's wrong because it fails to store the value in the dictionary, plus
> it always calculates the value rather than using the cached version --
> Python executes _calculate_value() even if the key *is* in the
> dictionary.
>
> I guess what I would really like is a version of cache.get() that
> takes a callable, so that this would work:
>
> <WAY_5>
> return cache.getOrElseCall(key, _calculate_value)
> </WAY_5>
>
> ...I guess maybe that can be achieved by slightly mis-using
> defaultdict, actually.
>
> This sort of thing is a common programming pattern in our code, so if
> someone has some good reference to read on the trade-offs, I'd be
> interested to read it. And I mean for this to be a discussion-y
> thread, so feel free to chime in if you have a preference.
>
> -- Asheesh.