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

[Asheesh Laroia]

(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.