Thanks a lot for the review Sascha, I will improve these things. <div class="gmail_quote">On Tue, Aug 21, 2012 at 9:38 AM, Sascha Silbe <<a href="mailto:silbe@activitycentral.com" target="_blank">silbe@activitycentral.com</a>> wrote: <blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">Martin Abente Lahaye <<a href="mailto:martin.abente.lahaye@gmail.com">martin.abente.lahaye@gmail.com</a>> writes: [src/jarabe/journal/volumestoolbar.py] [_set_up_activities_examples()] [...] <div class="im">> + activities = [] > + for home_activity in home_model._activities: > + if home_activity.is_journal(): > + continue > + > + activity_name = home_activity.get_activity_name() > + if activity_name not in activities: > + self._add_activity_example(home_activity) > + activities.append(activity_name) </div>I wonder if using a dictionary for deduplication would be better here. E.g.: activities = dict([(home_activity.get_activity_name(), home_activity) for home_activity in home_model]) for home_activity in activities.values(): self._add_activity_examples(home_activity) Alternatively, you could just iterate over all entries and let _add_activity_examples() take care of deduplication (as it has to do anyway), at the risk of making it O(n²). > + def _proper_occurrence(self, home_activity): What's a "proper" occurrence? <div class="im"> > + if home_activity.is_journal(): > + return False > + > + home_model = shell.get_model() > + act_name = home_activity.get_activity_name() > + activities = filter(lambda act: act_name == act.get_activity_name(), > + home_model._activities) > + > + return len(activities) <= 1 </div>I assume this is for deduplication again. The easiest and fastest way overall (O(1) per iteration [1], O(n) in total) is probably a ref-counting scheme: def __init__(self, ...): # ...the usual stuff... self._activity_counts = {} self._example_buttons = {} for home_activity in home_model: self._add_activity(home_activity) def _add_activity(self, home_activity): """Keep track of an active activity session Keep track of an active activity session and add the examples folder if available and not already shown. """ name = home_activity.get_activity_name() old_count = self._activity_counts.get(name, 0) self._activity_counts[name] = old_count + 1 if old_count: return # ...create button... self._example_buttons[name] = example_button def _remove_activity(self, home_activity): """Stop keeping track of an active activity session Remove all internal references to this session. Remove the corresponding examples folder if it's the last instance of this activity. """ name = home_activity.get_activity_name() old_count = self._activity_counts[name] self._activity_counts[name] = old_count - 1 if old_count > 1: return example_button = self._example_buttons.pop(name) # ...remove button... <div class="im"> > + def _add_activity_example(self, home_activity): > + examples_path = os.path.join(home_activity.get_bundle_path(), > + 'examples') > + > + if not os.path.exists(examples_path): > + return </div>If you add a guard against home_activity.get_bundle_path() returning None, you can drop the special-casing for the Journal activity. Sascha [1] <a href="http://wiki.python.org/moin/TimeComplexity#dict" target="_blank">http://wiki.python.org/moin/TimeComplexity#dict</a> -- <a href="http://sascha.silbe.org/" target="_blank">http://sascha.silbe.org/</a> <a href="http://www.infra-silbe.de/" target="_blank">http://www.infra-silbe.de/</a> </blockquote></div>