2

The following line is part of my code. I may be wrong, but it seems to be pythonic enough to me. However it is not clear, at first sight, what exactly it means. Is there a better code layout that would make it clearer? _idName is either a function or a DataFrame.

while  id1!="cancel" and ((id1 not in _idName.id.values) 
        if isinstance(_idName,_pd.DataFrame) else (_idName(id1) is None)):
    do something with the variables evaluated in the condition
jonrsharpe
  • 115,751
  • 26
  • 228
  • 437
bmello
  • 1,864
  • 3
  • 18
  • 23
  • possible duplicate of [Python style: multiple-line conditions in IFs](http://stackoverflow.com/questions/181530/python-style-multiple-line-conditions-in-ifs) – zopieux Aug 13 '15 at 12:13
  • 2
    Your code isnt currently correct python syntax – muddyfish Aug 13 '15 at 12:13
  • 2
    @muddyfish yes, it is! – jonrsharpe Aug 13 '15 at 12:18
  • PEP 8 is always a good place to start re syle questions about Python https://www.python.org/dev/peps/pep-0008/ – Levon Aug 13 '15 at 12:18
  • 1
    Well, I just learnt something them. But it looks so ugly so I won't use it – muddyfish Aug 13 '15 at 12:19
  • Also, how does it evaluate? – muddyfish Aug 13 '15 at 12:19
  • It's `while comparison and result_of_ternary:` – jonrsharpe Aug 13 '15 at 12:20
  • No, there is no missing `:` at the end of the `while`, the syntax is correct, there is an in-line `if` operator within the `while` predicate. @muddyfish What do you mean by "how does it evaluate"? – zopieux Aug 13 '15 at 12:21
  • @jonrsharpe I see missing `:` and some extra unneeded () -- if it's legal syntax, it seems unnecessarily obfuscated to me. – Levon Aug 13 '15 at 12:21
  • 1
    @Levon it is legal syntax, but I agree that it's a little obfuscated. And the brackets are necessary to get the appropriate order of evaluation. – jonrsharpe Aug 13 '15 at 12:21
  • 2
    The newline before the if is confusing. The best fix might be to use some intermediate variables or restructure so that it is obvious what the code does. – Joonazan Aug 13 '15 at 12:22
  • 1
    Zoupleux, it is not a duplicate because that question is about the "if" statement while my question is about the "if" operator. – bmello Aug 13 '15 at 12:23
  • could you refactor your code so that the type of _idName is known? – Joonazan Aug 13 '15 at 12:25
  • Is it equivalent to `while id1!="cancel" and {True: id1 not in _idName.id.values, False: _idName(id1) is None}[isinstance(_idName,_pd.DataFrame)]:`? – muddyfish Aug 13 '15 at 12:25
  • 2
    @muddyfish roughly, except that your version can't be lazily evaluated – jonrsharpe Aug 13 '15 at 12:25
  • @Jonnazan, I don't want to use intermediate variables because the variables evaluated in the condition are altered inside the while. – bmello Aug 13 '15 at 12:28
  • @bmello, you _could_ update them at the end, but that's not ideal. Although it is messy now; maybe you could create a small wrapper, e.g. `check_whatever(id1, _idName)`. Then `while check_whatever(id1, _idName):`. – Cyphase Aug 13 '15 at 12:31
  • Yes it is @muddyfish, although I have never seen that (nice) idiom. Is it preferable to the standard conditional operator? – bmello Aug 13 '15 at 12:31
  • 2
    @bmello no, it isn't preferable - it will (try to) evaluate **both** `id1 not in _idName.id.values` and `_idName(id1) is None`. – jonrsharpe Aug 13 '15 at 12:32

2 Answers2

3

The layout of your code does indeed make it pretty unclear what's going on.

At a minimum, I would be inclined to line break after the binary operators and and or, per the style guide, rather than in the middle of a single condition.

I would also try to keep a ternary on a single line, where possible; in this case that ends up being quite long, though, so probably not an option. I think the boolean operators in the ternary make more sense at the start of their line, although I can't find a reference for this (beyond "Martijn likes it too").

One more readable example:

while (id1 != "cancel" and 
       ((id1 not in _idName.id.values) if isinstance(_idName, _pd.DataFrame) 
        else (_idName(id1) is None))):

or perhaps:

while (id1 != "cancel" and 
       ((id1 not in _idName.id.values)
        if isinstance(_idName,_pd.DataFrame)
        else (_idName(id1) is None)):
Community
  • 1
  • 1
jonrsharpe
  • 115,751
  • 26
  • 228
  • 437
  • My expression actually isn’t equivalent, oops. – poke Aug 13 '15 at 12:46
  • @poke suggestion is indeed clever. However, I think the conditional operator makes it more clear that only one of the second clauses is evaluated, depending on the value of the condition. – bmello Aug 13 '15 at 12:49
  • No, the parens don’t do anything unfortunately (as that’s the normal operation order). The problem appears when `A` is true and `B` is false; in that case, `C` shouldn’t be evaluated (since `A` was true). – poke Aug 13 '15 at 12:58
  • You are write @poke, they are not equivalent. If A is True and B is false, it should return False and C should never be evaluated. – bmello Aug 13 '15 at 13:01
1

Create a small function that does the checking for you, then use that in the while statement.

def check_whatever(id1, _idName):
    if id1 == 'cancel':
        return False

    if isinstance(_idName,_pd.DataFrame):
        return id1 not in _idName.id.values:
    else:
        return _idName(id1) is None


while check_whatever(id1, _idName):
    do_stuff()
Cyphase
  • 11,502
  • 2
  • 31
  • 32
  • You can simplify this with e.g `return id1 not in _idName.id.values` – jonrsharpe Aug 13 '15 at 12:36
  • I'm not sure if that is simpler; it's just merging the last check with the fallback. This is easier to understand IMO (though it could probably be even easier). – Cyphase Aug 13 '15 at 12:38
  • We're working a bit blind since we don't know the semnatics behind all this. – Cyphase Aug 13 '15 at 12:39
  • It lets you turn six lines into three: `if isinstance(_idName, _pd.DataFrame): return id1 not in _idName.id.values; return _idName(id1) is None` is logically equivalent. – jonrsharpe Aug 13 '15 at 12:41
  • Yea, I just realized that as I was commenting about semantics; it's late :). – Cyphase Aug 13 '15 at 12:41
  • I agree with @jonrsharpe, it would be simpler and more legible. But I don't like to define a function just for this. – bmello Aug 13 '15 at 12:43