The Pythonic way of validating a long chain of conditions in Python

Question

So I have a long chain of conditions that should be validated to be true. Instead of chaining a long if condition, I tried to be "innovative" and did it this way, which I reckon is more readable. But my question is, is this the optimal way of doing it?

Or is there a pythonic way of doing it? PS: Please respond with an alternative instead of answering "No", thanks!

Here's the code chunk:

def site_exists(site):
    """
    returns the sitebean if it exists,
    else returns false
    """
    vpadmin_service = _get_vpadmin_service(site)
    all_sites = VpAdminServiceUtil.getSites(vpadmin_service)
    for site_listing in all_sites:
        if site.getId():
            #condition check
            try:
                assert site.getId() == site_listing.getId()
                assert site.getName() == site_listing.getName()
                assert site.getCustomer().getId() == site_listing.getCustomer().getId()
            except AssertionError:
                continue
            #pass conditions
            return site_listing
        #no id, so just check for name and customer
        else:
            #condition check
            try:
                assert site.getName() == site_listing.getName()
                assert site.getCustomer().getId() == site_listing.getCustomer().getId()
            except AssertionError:
                continue
            #pass conditions
            site.setId(site_listing.getId())
            return site_listing
    return False

in case one is wondering about the camel cases, its because i'm mixing some java methods in it. am using jython. — nubela, Mar 25 '11 at 11:08
Using assert and try-except might be performance hit. However, it's really minor thing, unless you have really huge number of sites. — Olli, Mar 25 '11 at 11:12
@Olli: nope. performance is not an issue here. would you suggest a better way of parsing a long chain of conditions instead thats readable and less performance inpeding? — nubela, Mar 25 '11 at 11:14
Why do you test site.getId each time through the loop? Should that be `if site_listing.getId()`? — Michael J. Barber, Mar 25 '11 at 11:16
@michael: there is an else clause that i did not include in the code chunk. EDIT: just revised the code chunk to incl everything. — nubela, Mar 25 '11 at 11:18
@nubela I guess I see the point, now. In the end, that's not very relevant, though -- it's just about the string of assertions, and we don't need to worry about side effects in the tests. — Michael J. Barber, Mar 25 '11 at 11:29
Note that the `assert` statement won't be executed if python is not run in debug mode. See http://docs.python.org/release/2.6.6/reference/simple_stmts.html#the-assert-statement — Don, Mar 25 '11 at 11:33

score 10 · Answer 1 · answered Mar 25 '11 at 11:40

A simpler approach is to build a tuple of the conditions and compare the tuples:

def site_info(s):
    return s.getId(), s.getName(), s.getCustomer().getId()

if site_info(site) == site_info(site_listing):
    return site_listing
else:
    continue

If you have a lot of conditions, or the conditions are expensive, you can instead create a generator for the conditions, and compare with any or all:

import itertools
def iter_site_info(s):
    yield s.getId()
    yield s.getName()
    yield s.getCustomer().getId()

if all(x==y for (x, y) in itertools.izip(iter_site_info(site), iter_site_info(site_listing)):
    return site_listing
else:
    continue

I'm not sure whether Jython has any and all, but they're trivial functions to write.

EDIT - any and all appeared in Python 2.5, so Jython should have them.

building a tuple of results may be less efficient since it will not short circuit - the functions getId, getName, getCustomer().getId will all be called every time, but with the other methods here functions after the first failure will be skipped. — Dave Kirby, Mar 27 '11 at 10:23
@Dave Kirby Yes, building the tuple does unnecessary work, which is why the generator version is there. Either way, it should be split off into a function, and a generator is a natural way to handle any expensive computations. — Michael J. Barber, Mar 27 '11 at 18:34

Sebastian Blask · Accepted Answer · 2011-03-25T14:19:52.873

6

Using exception for control flow is bad! And it will also kill your performance. Assuming the condition will be true in one out of 10 cases? So 9 exceptions to handle in the worst case - this comes with a huge performance cost.

If you want readability, try:

        if (
            condition1 and \
            condition2 and \
            condition3
            ):
            # do something

For the revised version, this should be equivalent:

for site_listing in all_sites:
    if site.getName() == site_listing.getName() and site.getCustomer().getId() == site_listing.getCustomer().getId():
        if not site.getId():
            site.setId(site_listing.getId())
        if site.getId() == site_listing.getId():
            return site_listing

edited Mar 25 '11 at 14:19

answered Mar 25 '11 at 11:13

Sebastian Blask

2,870
1
16
29

2

What are you talking about? I use exceptions for control flow all the time. And it'll jump to the `except` clause on the first exception thrown, so there's no efficiency issue. – Tom Zych Mar 25 '11 at 11:16
Sorry, forgot that, I added how I would do it. – Sebastian Blask Mar 25 '11 at 11:16
9

There's no need to use backslashes when the code is inside parentheses. – ThiefMaster Mar 25 '11 at 11:19
@Thiefmaster: true, but it helps my eyes ;-) – Sebastian Blask Mar 25 '11 at 11:31
Sebastian, you're probably german. But "contition" should be written "condition" ;-) – Alain Mar 25 '11 at 14:14
Indeed, but everybody produces typos? Fixed. – Sebastian Blask Mar 25 '11 at 14:21
4

@Sebastian Blask: `'\'` at EOL is ugly especially when it is not necessary. – jfs Mar 25 '11 at 16:39

Don · Answer 3 · 2011-03-25T11:43:29.047

I would implement an is_same method on site and call it in the for loops.

all_sites = VpAdminServiceUtil.getSites(vpadmin_service)
for site_listing in all_sites:
    if site_listing.is_same(site, set_id=True):
        return site_listing

As for its implementation (that is your real question), I suggest something like:

...
if self.getName()     != site.getName():     return False
if self.getCustomer() != site.getCustomer(): return False
...
return True

score 2 · Answer 4 · edited May 23 '17 at 10:28

Use the __eq__ method! If you wrote the class of site yourself, no problem. If it is from a module outside of your control, see this solution, or create a somewhat less elegant wrapping class. For the wrap variant, this would be the class:

class WrappedSite(object):
    def __init__(self, site):
        self.site = site

    def __eq__(self, other):
        if site.getId():
            if self.site.getId() != other.site.getId():
                return False
        if self.site.getName() != other.site.getName():
            return False
        if self.site.getCustomer().getId() != other.site.getCustomer().getId():
            return False
        return True

Then your big ugly function is reduced to this:

def site_exists(site):
    """
    returns the sitebean if it exists,
    else returns false
    """
    vpadmin_service = _get_vpadmin_service(site)
    all_sites = VpAdminServiceUtil.getSites(vpadmin_service)
    wsite = WrappedSite(site)
    for site_listing in all_sites:
        if wsite == WrappedSite(site_listing):
            return site_listing
    return False

EDIT Fixed site_exists to return sitebean instead of True.

You should really only provide your own implementation of __eq__ if there is a single, universally understood meaning of 'equality' for the given object. If you violate this rule, you risk encountering very strange behaviour. — blubb, Mar 25 '11 at 13:03
@Simon Good advice. I leave it to the poster to determine if this is a problem, as I'm not familiar with these `site` objects. To remedy the problem, if it is one, turn the `__eq__` method into a function `sites_are_the_same` (drop the wrapping class) and do `if sites_are_the_same(site, site_listing):` instead in the [antepenultimate](http://en.wiktionary.org/wiki/antepenultimate) line of `site_exists`. — Lauritz V. Thaulow, Mar 25 '11 at 13:45

jfs · Answer 5 · 2011-03-25T12:28:46.797

Remove some code duplication:

def site_exists(site):
    """
    returns the sitebean if it exists,
    else returns None
    """
    vpadmin_service = _get_vpadmin_service(site)
    all_sites = VpAdminServiceUtil.getSites(vpadmin_service)

    for site_listing in all_sites:
        if (site.getName() == site_listing.getName() and
            site.getCustomer().getId() == site_listing.getCustomer().getId()):
            if site.getId(): # if id is set; it should be the same
                if site.getId() != site_listing.getId(): continue
            else: # no id; consider it the same site
                site.setId(site_listing.getId()) #XXX side-effect
            return site_listing

Note: it is unexpected that site_exists() might modify its argument (via .setId()). Consider to refactor it:

def same_site(site, other):
    if site.getId() and site.getId() != other.getId(): 
       # if id is set; it should be the same
       return False 
    return (site.getName() == other.getName() and
            site.getCustomer().getId() == other.getCustomer().getId())

def get_site_listing(site):
    """
    returns the sitebean corresponding to `site`,
    returns None if there is none
    """
    vpadmin_service = _get_vpadmin_service(site)
    all_sites = VpAdminServiceUtil.getSites(vpadmin_service)

    return next((s for s in all_sites if same_site(site, s)), None)

Note: the code doesn't modify the site object. Use the return value from get_site_listing() instead.

If next() is unavailable then use:

for site_listing in all_sites:
    if same_site(site, site_listing):
       return site_listing
return None

btw, jython should provide property wrappers for you; so you could write:

def same_site(site, other):
    if site.id and site.id != other.id: 
       # if id is set; it should be the same
       return False 
    return site.name == other.name and site.customer.id == other.customer.id

and site.id = id instead of site.setId(id).

score 0 · Answer 6 · answered Mar 25 '11 at 19:36

0

Using an exception handler as a code target is basically a "goto". I don't see anything wrong with it really (despite the ancient furor surrounding "goto considered harmful"), but knowing that it's a goto might give you a different perspective.

answered Mar 25 '11 at 19:36

Chris McDonough

2,479
18
18

score 0 · Answer 7 · answered Mar 26 '11 at 06:19

Using exception handling for control flow is pythonic in the sense that "it's easier to ask for forgiveness than for permission". However, that doesn't extend to making rules to break just so that you can ask for forgiveness.

Others have provided excellent alternatives. Just wanted to address the principle aspect.

The Pythonic way of validating a long chain of conditions in Python

7 Answers7