veky
on Nov. 1, 2016, 3:33 a.m.
<p>There is really no reason for a separate function. Lines 10~22 should just be up there, instead of line 4.</p>
<p>Also, instead of naming the result (with those unfortunate parentheses), it would be better to return it immediately.</p>
<pre class='brush: python'>if f_result is g_result is None: return None, 'both_error'
</pre>

siebenschlaefer
on Dec. 20, 2016, 10:15 a.m.
<p>If some chunk of code should be a put into a function is a matter of taste. In "Clean Code" Robert C. Martin advocates for small functions that do only one thing and have only one level of abstraction. The Clean Code followers like to put the body of a loop, a try statement, a condition, nearly everything into its own function. On the other hand, many open source C projects are composed of large functions with often 100 lines or more.</p>
<p>Returning immediately is also a matter of taste. Edsger W. Dijkstra's Structured Programming forbade early exits, others allowed it. Some code metrics tools still punish you for too many exits. </p>
<p>I agree to your third point. For some strange reason I did not think of the ``is`` keyword as an operator. Thanks, I will do that from now on.</p>

veky
on Dec. 21, 2016, 7:02 a.m.
<p>First: Even if I agreed with Uncle Bob (which I don't generally, but that's not the point here), this is <em>not</em> what we are talking about here. You already <em>have</em> your small function that does only one thing (really disputable here, but you said so in implementing status<em>for</em>results) and has only one level of abstraction (probably). <em>It is called h</em> (or at least you called it so).</p>
<blockquote>
<p>followers like to put the body of a loop, a try statement, a condition, nearly everything into its own function. </p>
</blockquote>
<p>You'll notice that the list above doesn't include "another function definition". :-D Otherwise you'd have a nice conundrum about where to stop. :-D</p>
<pre class='brush: python'>def f():
return g()
def g():
return h()
def h():
do some actual work
return the result
</pre>
<p>It's the same logical problem as advocating always having a relational operator in your conditions, making you write `if condition is True:` instead of `if condition:`. Same thing: why not write `if (contition is True) is True` then? :-D</p>
<hr />
<p>Second: ah, Dijkstra. Don't make me count all the things he has advocated. :-) Yes, he was a genius, but he was working while his field was extremely primitive. Einstein probably wouldn't envision special relativity in an Amazon tribe. :-D SESE is an abandoned philosophy nowadays. And besides, in <em>this</em> case all it gives you is shifting the branching around. You still have the same cyclomatic complexity (even bigger by some measuring tools).</p>
<p>And you know what they say about metrics. Tools always measure what's easy to measure, not what you actually want tracked.</p>
<hr />
<p>Third: it's interesting to know that `in` is also in the same category, though used chainly much rarer.</p>
<pre class='brush: python'>if 'do' in my_input in possible_inputs: ...
</pre>

siebenschlaefer
on Dec. 22, 2016, 1:40 a.m.
<p>Yes, you are right about your first point. I still think that your second point is more a matter of style. I submitted another version. Thank you for your review.</p>

veky
on Dec. 22, 2016, 3:15 a.m.
<p>Of course it is a matter of style. I'm just saying that it has disadvantages that <em>in this example</em> outweigh the benefits. When reading your code, a reviewer must branch <em>and</em> recollect all the branches together at the end. Of course, they can just trust you that since you named the object 'result', it <em>is</em> the intended return value, but still they have to check whether you're doing some postprocessing on it later. If you return immediately, they don't have to check anything later: Python ensures it <em>will</em> be the return value (absent weird cases like finally:;).</p>
<p>In my view, it is the same as</p>
<pre class='brush: python'>index = 0 index = 0
while index &lt; len(array): vs. while index &lt; len(array):
if array[index] == sought: if array[index] == sought:
print(index) print(index)
break index = len(array)
else: else:
index += 1 index += 1
</pre>
<p>I'm sure you'll agree that there is value in directly breaking the loop, instead of roundaboutly breaking it by just going directly to the end. In the first instance, I can just continue reading the code, since I know `break` will actually exit the loop. In the second, I must still make one more visit to the top, just to check whether that rebinding of the index will actually exit the loop.</p>