views:

192

answers:

3

I have this piece of code (should be self-explanatory; if not, just ask):

for tr in completed_taskrevs:
    found = False
    for nr in completion_noterevs:
        if tr.description in nr.body:
            completion_noterevs.remove(nr)
            found = True
            break
    assert found

How can I make it more pythonic?

+8  A: 

Try this:

for tr in compleded_taskrevs:
    try:
        nrs = (nr for nr in completion_noterevs if tr.description in nr.body)
        completion_noterevs.remove(nrs.next())
    except StopIteration:
        raise ValueError('Some error')

EDIT: Devin is right. Assertion is not the way to go, better to use a standard exception.

gruszczy
Thank you. It probably looks most Pythonic, but it's not obvious for me on the first sight. That's why I'm now voting for your answer but accepting Devin's one.
Tomasz Zielinski
+3  A: 

The list generator below could return you all items nr that are valid tr.description in nr.body however it's hard to simplify your algorithm as it has quite a few branches. I would just say go with the algorithm you have as long as it does what you want.

[nr for tr in completed_taskrevs for nr in completion_noterevs if tr.description in nr.body]
ruibm
This also looks Pythonic, but it's too obfuscated for me. I prefer both Pythonic and easily readable code. Writing this, vote up for you.
Tomasz Zielinski
Yup, I do agree with the obfuscation bit. One must always seek for balance.
ruibm
+5  A: 

Using assert/AssertionError is probably wrong here, I'd say. It's useful for "debugging assertions", which is to say, to make sure your code is sane. It's not useful as a way to raise an error when you get invalid data, for many reasons, the most interesting of which is probably that assert isn't even guaranteed to be executed-- if your code is compiled with any optimization settings, it won't be. And heck, even if this is a debugging thing, I'd still use raise-- it's more readable, and it'll always happen, no matter when or why the data is wrong.

So to make it more "pythonic", I would remove the assert and replace it with something nicer. As it happens, such a nicer thing exists, and it's the raise statement. Further, I would replace the clumsy value set/check with the else clause of loops, which is executed when the loop is exhausted (or, for while loops, when the condition becomes false). So if you break, the else clause is not executed.

for tr in completed_taskrevs:
    for nr in completion_noterevs:
        if tr.description in nr.body:
            completion_noterevs.remove(nr)
            break
    else:
        raise ValueError("description not found"); # or whatever exception would be appropriate

Other than this, I probably wouldn't change anything.

Devin Jeanpierre
That 'else' is very nice, I didn't know about it.Regarding assertion - code excerpt was taken from quickly hacked tool, and not production code, therefore for me (and my customer) it's perfectly acceptable to use assertion.
Tomasz Zielinski