-1

I'm a relative python beginner, now fairly comfortable with the language but still grappling over issues of what's "pythonic" and not. I was wondering what people's thoughts on this issue are.

For example, take this line of code for calculating the average cost per week of rental properties drawn from a peewee database:

    rental_avg_costperweek = sum([calcCostPerWeek(rental.price, rental.rental_freq) for rental in
                                 [LLSRental.select(LLSRental.id == this_id) for this_id in closest_rental_ids]]) \
                             / len(closest_rental_ids)

It uses nested list comprehensions, which might be confusing.

Alternatively I could stick the inner comprehension into a temporary variable:

    closest_rental_records = [LLSRental.select(LLSRental.id == this_id) for this_id in closest_rental_ids]
    rental_avg_costperweek = sum([calcCostPerWeek(rental.price, rental.rental_freq) for rental in closest_rental_records]) \
                             / len(closest_rental_ids)

This might be (a little) easier to read, but being an ex-C++ programmer I have an aversion to creating temporary variables purely for readability, as it clutters the namespace and potentially makes more work for the garbage collector.

Also I think if a reader doesn't understand the variable is purely temporary it may make the code more confusing if there are many such variables.

As such, I'm inclined to go with the first option over the second despite the guidance of "flat is better than nested"...but what do ye python veterans think?

Thanks!
gs.

Guy Smiley
  • 141
  • 1
  • 8
  • 5
    "I have an aversion to creating temporary variables purely for readability" -- You should get over that. It doesn't make more work for the GC because if you create the object, it needs to be collected no matter what. If you really want to make sure that the temporary variable is collected right away, there's `del` which will decrease the ref-count and make it available for collection. – mgilson Apr 03 '13 at 12:11
  • 1
    Your functions may be doing too much work if you need to worry about cluttering the local namespace. – Janne Karila Apr 03 '13 at 12:14
  • This question is not constructive, there is no right answer. The only thing here is preference, and requirements. You could use [PEP8](http://www.python.org/dev/peps/pep-0008/), or you could be forced to follow a certain style from your place of employment or education... otherwise... preference. – Inbar Rose Apr 03 '13 at 12:25
  • Also **never** use `\ ` to continue a line. Simply use parentheses around the expression. An other observation I can make, regarding the more work for the GC, is that you are creating a `list` and pass it to `sum` when a genexpr would suffice and avoid the extra memory alloc/deallocation – Bakuriu Apr 03 '13 at 12:26
  • @Inbar What's the harm in asking for people's opinions on style issues? – Guy Smiley Apr 03 '13 at 12:26
  • SO isn't intended for people to query opinions on issues like this, see http://stackoverflow.com/faq#dontask – bgporter Apr 03 '13 at 12:33
  • I'm pretty sure this has been asked before, can't seem to find the question – jamylak Apr 03 '13 at 12:40
  • @Bakuriu Thanks for your tips esp. re: generator expressions. – Guy Smiley Apr 03 '13 at 14:36

1 Answers1

1

I think both are wrong. It looks like you're doing the inner comprehension because you want to avoid recalculating LLSRental.select(). You should rarely need inner comprehensions if you're using comprehensions appropriately, because you can nest them, like

all_the_inputs = [ process_value(x) for y in all_the_stuff for x in y ]

or something. This is a nice but short post which explains this well.

Anyway. Something like

rental_avg_costperweek = 0
for this_id in closest_rental_ids:
    rental = LLSRental.select(LLSRental.id == this_id)
    rental_avg_costperweek += calcCostPerWeek(rental.price, rental.rental_freq)
rental_avg_costperweek /= len(closest_rental_ids)

seems like a much more appropriate way to do the calculation. I'm actually creating fewer objects than your code does, because I don't have two lists I make for no good reason, whereas your code makes two lists for intermediate calculations and then throws them away.

desfido
  • 787
  • 6
  • 16
  • Thanks for your answer. Thanks for the insight that my code was creating unnecessary objects, and also for the double-loop form of the list comprehension, which is extremely useful. I think I've been erroneously using lists comprehensions as loops in a effort to create succinct code, when in fact doing so can actually be more inefficient as you pointed out. – Guy Smiley Apr 03 '13 at 14:33