Code Review Stack Exchange is a question and answer site for peer programmer code reviews. Join them; it only takes a minute:

Sign up
Here's how it works:
  1. Anybody can ask a question
  2. Anybody can answer
  3. The best answers are voted up and rise to the top

I have a case in which I need to determine whether a for loop yielded at least one element:

@property
def source_customers(self):
    """Yields source customers"""
    for config in RealEstatesCustomization.select().where(
            RealEstatesCustomization.customer == self.cid):
        yield config.source_customer
    # Yield self.customer iff database
    # query returned no results
    try:
        config
    except UnboundLocalError:
        yield self.customer

Is this a pythonic way to go or shall I rather use a flag for that like so:

@property
def source_customers(self):
    """Yields source customers"""
    yielded = False
    for config in RealEstatesCustomization.select().where(
            RealEstatesCustomization.customer == self.cid):
        yield config.source_customer
        yielded = True
    # Yield self.customer iff database
    # query returned no results
    if not yielded:
        yield self.customer

What is preferable?

share|improve this question
    
What would it mean for the caller if source_customers returned self.customer? Isn't that weird? – Barry Dec 7 '15 at 14:56
    
The property yields customers for the real estate filter. Those customers can be specified within an appropriate database configuration represented by the peewee based ORM RealEstatesCustomization. If no deviating customer(s) is specified within the database configuration, the real-estate customer is assumed to be the instance's customer. – Richard Neumann Dec 7 '15 at 16:07
1  
@RichardNeumann Hi! Thanks for accepting my answer, but you may want to wait a bit for other people before you accept. Sometimes answers on Code Review take a while, and people can be dissuaded from reviewing when they seen an accepted answer. I generally recommend waiting at least a day. You can unaccept my answer if you wish, the decision is up to you. – SuperBiasedMan Dec 7 '15 at 16:25
up vote 3 down vote accepted

Separate Your Concerns

The function at the moment does two things: finds matching customers and checks that it's nonempty. Let's instead add a default checker:

from itertools import tee

def iter_or_default(iterable, dflt):
    orig, test = tee(iter(iterable))
    try:
        next(test)
        # if we're still here, the iterable is non-empty
        yield from orig
    except StopIteration:
        # iterable was empty
        yield dflt

And now, we just use that:

@property
def source_customers(self):
    """Yields source customers, or the instance customer if none found"""

    source_customers = (c.source_customer for c in 
         RealEstatesCustomization.select().where(
            RealEstatesCustomization.customer == self.cid))
    yield from iter_or_default(source_customers, self.customer)
share|improve this answer
    
After some further thought I added a variant of your proposal to my library: (github.com/HOMEINFO/homeinfo-lib/blob/master/homeinfo/lib/…) – Richard Neumann Dec 8 '15 at 11:08

This is very strange to read:

try:
    variable
except UnboundLocalError:

I'm familiar with NameErrors but didn't realise UnboundLocalError was a separate exception. It's generally confusing to have a variable sitting by itself doing nothing, though I know why you're doing it in this case. I think your yielded boolean makes far more sense because if not yielded makes perfect sense to read.

To make it extra explicit, your docstring should contain the note that even if no value from RealEstatesCustomization is yielded the generator will at least fall back on self.customer.

@property
def source_customers(self):
    """Yields source customers

    Yield self.customer if database query returned no results."""

    yielded = False
    for config in RealEstatesCustomization.select().where(
            RealEstatesCustomization.customer == self.cid):
        yield config.source_customer
        yielded = True

    if not yielded:
        yield self.customer
share|improve this answer

I'm coming back to this question as I'm not entirely satisfied with either answer or your original code. And I think the main part that is bothering me is the simple fact that you hide away an error situation.

I think that both ways of detecting that the loop hasn't executed seems sensible, although the UnboundLocalError is a bit of a hack, but to me the main issue that you void the separation of concern by returning something when you don't find anything.

This should be done in the outer code, in my meaning, and as such it would be proper to raise an exception instead of returning/yielding self.customer. So the last part of your code I think I would use something like:

raise NoCustomizationFound(self.customer)

Which would allow for the outer level to take proper action when no customization is found, and you could (as suggested but not tested) include the self.customer as a parameter to the exception and thusly allow the outer code to continue using that as the value.

If not moving this check to the outer level, where it seems natural to have it, you could opt for doing it the way Barry suggests since you then indicate at the outer level that this error situation is handled by the iter_or_default() part. But this does still hide the functionality somewhat, as when you use the property you don't really know if you got the customization you are looking for, or the default value.

share|improve this answer
    
I understand your concerns. However the caller of the property does not know (cue: information hiding) that there is a database for configuring deviating customers than the one of the instance as real estate owners. All it wants is to get the real estate related customers, no matter where they come from. The "yield or default" mechanism in this case is so to speak an internal operation of the propertie's class and must not concern the caller. – Richard Neumann Dec 8 '15 at 16:50

Here is a cleaner version of yield_or_default created by @Barry and updated by @RichardNeumann

def yield_or_default(iterable, defaults):
    """Yields from the iterable or yields
    from the defaults iff iterable is empty
    """
    iterator = iter(iterable)
    try:
        yield next(iterator)
    except StopIteration:
        # Iterable is empty
        yield from defaults
    else:
        yield from iterator

Why I believe it is Cleaner:


In comparison to:

def yield_or_default(iterable, defaults):
    """Yields from the iterable or yields
    from the defaults iff iterable is empty
    """
    iterator = iter(iterable)
    try:
        first = next(iterator)
    except StopIteration:
        # Iterable is empty
        yield from defaults
    else:
        yield first
        yield from iterator

The 1st solution is 1 less statement, why save to first variable when you can simply attempt to yield, catch exception, and either yield from defaults or yield from iterator (to get the rest of items in iterator).

share|improve this answer
    
You should explain why it's cleaner IMO – syb0rg Dec 10 '15 at 3:41
1  
@syb0rg Thanks I have added explanation and comparison on why I believe it is cleaner – jamylak Dec 10 '15 at 3:51
    
@jamylak Thanks, but this goes into code golf. Furthermore it's a habit of mine to not use return, yield and such in cases where I except for a specific error, but give the anticipated value a name and handle it in the else or finally block. This may produce more LOC, but I find it convenient and do not want to deviate from this habit. – Richard Neumann Dec 10 '15 at 9:24
    
@RichardNeumann I disagree but I understand your perspective – jamylak Dec 10 '15 at 23:34

Yet another variation, no 'tee', no 'else'.

def iter_or_default(iterable, dflt):
   it = iter(iterable) 
   try:
      yield next(it)
      # here iterable wasn't empty,
      # we yield remaining items
      yield from it
   except StopIteration:
      yield dflt
share|improve this answer

Your Answer

 
discard

By posting your answer, you agree to the privacy policy and terms of service.

Not the answer you're looking for? Browse other questions tagged or ask your own question.