On Mon, Sep 9, 2019 at 4:13 AM Rob Cliffe via Python-ideas
On 07/09/2019 18:59:49, Chris Angelico wrote:
On Sat, Sep 7, 2019 at 11:27 PM Rob Cliffe via Python-ideas
wrote: A chance for me to bang the drum on one of my pet themes: Sometimes the readability of code is improved by breaking the sacred taboo of 1 statement per line, if it allows similar constructs to be vertically aligned:
if max_results > 0 : query['max_results'] = max_results if active is not None : query['active'] = active if deleted is not None: query['deleted'] = deleted
If this comes out ragged in your browser, the colons and equal signs are meant to be vertically aligned. I'm not bothered by putting an if and a simple statement together, but I am definitely bothered by the need to vertically align them, and especially by the triple repeated name. Not a fan of this style - looks like a bug magnet and maintenance burden.
ChrisA The OP's code:
if max_results > 0: query['max_results'] = max_results if active is not None: query['active'] = active if deleted is not None: query['deleted'] = deleted
is perfectly reasonable and has the same triple repeated name. Would you rewrite it differently, and if so, how? I'm just saying that aligning similar elements and bringing them together, without intervening lines, makes it easier to see the code structure and spot typos. Not to mention using less vertical screen space. I don't understand why you think it is a bug magnet and maintenance burden. I think it is *easier* to maintain: in which of the following is it easier to spot the mistake?
# Version 1: if max_results > 0 : querydata['max_results'] = max_results if active is not None : querydata['active'] = active if deleted is not None: quervdata['deleted'] = deleted
# Version 2: if max_results > 0: querydata['max_results'] = max_results if active is not None: querydata['active'] = active if deleted is not None: quervdata['deleted'] = deleted
Data point: I have used this style repeatedly in my code (only where appropriate, of course) and find it helpful.
Now add a fourth one, for result_count. What does your diff look like? Do you have to edit every other line just to realign it? That's the maintenance burden - fiddling around with formatting. Honestly, I couldn't see the mistake in _either_ version until I looked pretty closely, upon which it was equally visible in both. If I were using this in a full app, it would be something a linter could easily spot, again regardless of the alignment. You're right that the triple repeated name is not caused by your style. However, your suggestion also doesn't solve it. What I'd probably want to do, here, is build the dictionary with everything, and then have a separate pass that removes anything that's None (or maybe "any of these keys, if the value is None"), and then just special-case the requirement for max_results to be positive. But I'd be looking to make a broader change somewhere, taking into account a lot more code, rather than doing this whole "take local names and plonk them into a dict" thing at all. ChrisA