Take the 2-minute tour ×
Code Review Stack Exchange is a question and answer site for peer programmer code reviews. It's 100% free, no registration required.

In the code below, I have a dictionary of attributes and values. I am checking to see if the value if an attribute is True, and if so adding the attribute to a list for later processing (it becomes part of a SQL group by).

This code works fine, but I was wondering how it could be more "Pythonic". Note that the col_map dict is actually a lot bigger.

   col_map = {      
        'dim_3a_group_checkbox': 'pn_group',
        'dim_3a_family_checkbox': 'pn_family',
        'dim_3a_root_checkbox': 'pn_root',
    }
    # get an object with the attributes (eg:dim_3a_root_checkbox) as True / False
    supply_kpi = self.browse(cr, uid, ids, context=None)[0]
    group_by = []
    for key, value in col_map.iteritems():
        x = getattr(supply_kpi,key)
        if x:   # only add if attribute is True
            group_by.append(value)
    print group_by

Example results

    ['pn_family', 'order_type', 'pn_group', 'categ']
share|improve this question

2 Answers 2

up vote 6 down vote accepted

I would do

supply_kpi = self.browse(cr, uid, ids, context=None)[0]
group_by = [col_map[key] for key in col_map if getattr(supply_kpi, key)]
  • Iterating with list comprehension is much more efficient.
  • You don't build iteritems list of tuples.
share|improve this answer
    
Cf @sundar's answer to avoid calling cal_map[key] for each key. –  Josay Jul 18 at 10:35
2  
@Josay is using iteritems() preferred over cal_map[key] ? –  SeanGeneva Jul 18 at 11:06
1  
It is indeed. That's exactly what iteritems() is for. The iterator provides you the keys and values instead of the just the keys so that you don't have to retrieve the values yourself. More about this on Raymond Hettinger's talk at pycon US 2013 : video : youtube.com/watch?v=OSGv2VnC0go / slides : speakerdeck.com/pyconslides/… –  Josay Jul 18 at 12:48
1  
using iteritems is fine. There is a little cost of going through the generator and building the tuples. I would say around 15% from what I see ... The look-up in the dictionary has a big cost but depends also how often getattr(supply_kpi, key) is true. Doing col_map[key] can actually be faster. –  Ugo Jul 18 at 13:05

total groupby can be written in single line list comprehension

groupby=  [value  for key, value in col_map.iteritems() if getattr(supply_kpi,key) ]
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.