On Mon, Sep 9, 2019 at 4:13 AM Rob Cliffe via Python-ideas <python-ideas@python.org> wrote: > > On 07/09/2019 18:59:49, Chris Angelico wrote: > > On Sat, Sep 7, 2019 at 11:27 PM Rob Cliffe via Python-ideas > > <python-ideas@python.org> 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 _______________________________________________ Python-ideas mailing list -- python-ideas@python.org To unsubscribe send an email to python-ideas-le...@python.org https://mail.python.org/mailman3/lists/python-ideas.python.org/ Message archived at https://mail.python.org/archives/list/python-ideas@python.org/message/65SSURULSQU27F2KVUYCESWAWUTRNB4O/ Code of Conduct: http://python.org/psf/codeofconduct/
