Multiple Python decorators when using "replacing" functions

Question

Perhaps what I'm trying to do just doesn't work. I also assume that there are other (better?) ways to accomplish the same thing, but what I'm trying to do is:

I have a class that will have several (at least 4, possibly more) lists of dictionaries. I want to set up a single "load" function and a single search function that can be used for all of them. If any of the "load" or search functions is called, and the list is empty, the list should be loaded.

The methods for loading them are each different and have different args. The search function needs to allow for searching on different members of the dictionaries that make up the list, and for returning either the entire matching dictionary or specific members.

I have been using a decorator to specify the search function ala:

@search_list
def get_name(self,id):
    pass

So that I effectively replace the function with the search, and can use the func.__name__ in the search function to determine which list from the class to use and what member, if any (_xx on the end of the function name), to return.

The problem I'm having is that I can use the load function, get_list(), by itself when doing something like:

@get_list
def get_names(self):
    return self.names

But I have not been able to get it to load the list if the first thing I call is one of the search functions. If I call one of the @get_list functions, then the search function works fine after that, but I don't want to call a load function unless someone using the module wants that particular list. All of the lists are loaded via REST calls and some take a while.

Like I said, there may be other (better) ways to accomplish this, but this has been driving me crazy...it's a short trip :-)

Any suggestions would be appreciated.

Here's the sample code I've been using:

#!/usr/bin/env python

import re
from functools import wraps

NAMES = [
         {'name':'Fred Flintstone','id':1},
         {'name':'Barney Rubble','id':2},
         {'name':'Henry Ford','id':3},
         ]

def get_list(func):
    @wraps(func)
    def wrapper(self,*args):
        res = re.match('get_(?P<list>[^_]+)(_(?P<return>.*))*',func.__name__)
        if res:
            list_name = res.group('list').endswith('s') and res.group('list') or (res.group('list') + 's')
            if not eval("self.%s" % list_name):
                exec "self._Fred__get_%s()" % list_name in globals(),locals()
        return func(self,*args)
    return wrapper

def search_list(func):
    @wraps(func)
    def wrapper(self,*args):
        res = re.match('get_(?P<list>[^_]+)(_(?P<return>.*))*',func.__name__)
        if res:          
            for x in eval('self.%ss' % res.group('list')):
                if isinstance(args[0],(int,long)):
                    if x['id'] == args[0]: 
                        return res.group('return') and x[res.group('return')] or x
                else:
                    if x['name'] == args[0]:
                        return res.group('return') and x[res.group('return')] or x
    return wrapper

class Fred(object):
    def __init__(self):
        self.names = []

    def __get_names(self):
        self.names = NAMES

    @get_list
    def get_names(self):
        return self.names

    @get_list
    def get_name_names(self):
        return [x['name'] for x in self.names]

    @search_list
    @get_list
    def get_name(self,id):
        pass

    @search_list
    def get_name_name(self,id):
        pass

    @search_list
    def get_name_id(self,name):
        pass

Answer 1

So first of all, decorators are meant to be generic things which can be applied to a larger group of things. The decorators you came up with are extremely specific to your Fred class to the point where it makes no sense to have them defined even outside of it (The name Fred is even hardcoded into them).

What’s even worse, the decorators are very hacky, accessing locals(), globals(), and using eval to execute dynamic code. None of that is a good idea. Also, the way you build upon the names of the function make the logic highly dependent on the public interface of the class—or the other way around, the public interface is highly dependent on the implementation details of your thing. get_name_names is definitely not a good function name.

Next, I don’t even know what the purpose of this is. Overall, you seem to want some kind of lazy loading, as you get the data from elsewhere via REST. That’s fine, but doesn’t judge this kind of language abusing. The way you are hiding the actual lookup logic in those decorators makes it impossible to understand what’s going on, making maintenance extremely hard.

Instead, it would be more than fine if you just duplicated a bit inside the functions but had a clear and simple logic flow that way. It’s likely that the program will run faster that way too.

If you want to implement lazy-loading nicely, you could try properties. That could look like this:

import time
NAMES = [
    {'name': 'Fred Flintstone', 'id': 1},
    {'name': 'Barney Rubble', 'id': 2},
    {'name': 'Henry Ford', 'id': 3}
]

class Fred (object):
    def __init__ (self):
        self._names = []

    @property
    def names (self):
        if not self._names:
            print('Load the data here; might take a while.')
            time.sleep(5)
            self._names = NAMES
        return self._names

    def getNames (self):
        return [x['name'] for x in self.names]

    def searchNameById (self, id):
        return filter(lambda x: x['id'] == id, self.names)

    def searchNameByName (self, name):
        return filter(lambda x: x['name'] == name, self.names)


f = Fred()

print('Querying some stuff:')
print(f.searchNameById(2))
print(f.searchNameByName('Fred Flintstone'))
print(f.names)
print(f.getNames())

Executed, it yields this result. Note that the data is only loaded once:

Querying some stuff:
Load the data here; might take a while.
[{'name': 'Barney Rubble', 'id': 2}]
[{'name': 'Fred Flintstone', 'id': 1}]
[{'name': 'Fred Flintstone', 'id': 1}, {'name': 'Barney Rubble', 'id': 2}, {'name': 'Henry Ford', 'id': 3}]
['Fred Flintstone', 'Barney Rubble', 'Henry Ford']

And I’d argue that this is a lot clearer than your decorator stuff.

Answer 2

Regular expressions parsing method names, double-underscore name mangling and de-mangling, eval, exec, and/or attempts at a ternary operator (protip: Python has a ternary operator: foo if condition else bar)... Apologies in advance, but that's fantastically bad design.

In addition, your search_list decorator never calls the original function, meaning any function you decorate with it will always return None.

What it looks like you're actually trying to do is lazy loading -- that is, the first time any function tagged with get_list or search_list is called, you want to execute __get_names to make it load stuff from the network/disk/whatever (additional note: get_names is a really bad name for such a method, because people who read your code will assume it's a getter that has no side effects, when it's precisely the opposite -- call it load_names instead).

Here's how I would approach such a design:

from functools import wraps

NAMES = [
    {'name':'Fred Flintstone','id':1},
    {'name':'Barney Rubble','id':2},
    {'name':'Henry Ford','id':3},
]


class lazy_load(object):
    def __init__(self, cache_name):
        self.cache_name = cache_name

    def __call__(self, func):
        @wraps(func)
        def wrapper(instance, *args):
            # instance is the instance of the class whose method we decorated,
            # whereas self is the instance of the decorator. Tricky ;)
            if self.cache_name not in instance.cache:
                instance.load_cache(self.cache_name)
            return func(instance, *args)
        return wrapper


class Fred(object):
    def __init__(self):
        self.cache = {}

    def load_cache(self, name):
        # TODO Add actual logic to load the data here.
        self.cache[name] = NAMES

    @lazy_load("names")
    def get_names(self):
        return self.cache["names"]

    @lazy_load("names")
    def get_name_by_id(self, id):
        # Such iteration is also terrible design.
        # If you expect access by ID to be common, you want a big {id: name_entry} dict,
        # not a list.
        for i in self.cache["names"]:
            if i["id"] == id:
                return i