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

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
    print group_by

Example results

    ['pn_family', 'order_type', 'pn_group', 'categ']
share|improve this question
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 '14 at 10:35
@Josay is using iteritems() preferred over cal_map[key] ? – SeanGeneva Jul 18 '14 at 11:06
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 : / slides :… – Josay Jul 18 '14 at 12:48
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 '14 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


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.