Is it bad to store all instances of a class in a class field?

Question

I was wondering if there is anything wrong (from a OOP point of view) in doing something like this:

class Foobar:
    foobars = {}
    def __init__(self, name, something):
        self.name = name
        self.something = something

        Foobar.foobars[name] = self

Foobar('first', 42)
Foobar('second', 77)

for name in Foobar.foobars:
    print name, Foobar.foobars[name]

EDIT: this is the actual piece of code I'm using right now

from threading import Event
class Task:
    ADDED, WAITING_FOR_DEPS, READY, IN_EXECUTION, DONE = range(5)
    tasks = {}
    def __init__(self, name, dep_names, job, ins, outs, uptodate, where):
        self.name = name
        self.dep_names = [dep_names] if isinstance(dep_names, str) else dep_names
        self.job = job
        self.where = where
        self.done = Event()
        self.status = Task.ADDED
        self.jobs = []
        # other stuff...
        Task.tasks[name] = self
    def set_done(self):
        self.done.set()
        self.status = Task.DONE
    def wait_for_deps(self):
        self.status = Task.WAITING_FOR_DEPS
        for dep_name in self.dep_names:
            Task.tasks[dep_name].done.wait()
        self.status = Task.READY
    def add_jobs_to_queues(self):
        jobs = self.jobs
        # a lot of stuff I trimmed here
        for w in self.where: Queue.queues[w].put(jobs)
        self.status = Task.IN_EXECUTION
    def wait_for_jobs(self):
        for j in self.jobs: j.wait()
    #[...]

As you can see I need to access the dictionary with all the instances in the wait_for_deps method. Would it make more sense to have a global variable instead of a class field? I could be using a wrong approach here, maybe that stuff shouldn't even be in a method, but it made sense to me (I'm new to OOP)

Shipp

If you decide to keep doing this, I'd suggest you to use self.foobars or type(self).foobars so that the class isn't hardcoded. The latter requires new-style classes (i.e. classes that inherit from object). – Varices

Answer 1

Yes. It's bad. It conflates the instance with the collection of instances.

Collections are one thing.

The instances which are collected are unrelated.

Also, class-level variables which get updated confuse some of us. Yes, we can eventually reason on what's going on, but the Standard Expectation™ is that state change applies to objects, not classes.

---

 class Foobar_Collection( dict ):
     def __init__( self, *arg, **kw ):
         super( Foobar_Collection, self ).__init__( *arg, **kw ):
     def foobar( self, *arg, **kw ):
         fb= Foobar( *arg, **kw )
         self[fb.name]= fb
         return fb

 class Foobar( object ):
     def __init__( self, name, something )
         self.name= name
         self.something= something

fc= Foobar_Collection()
fc.foobar( 'first', 42 )
fc.foobar( 'second', 77 ) 

for name in fc:
    print name, fc[name]

That's more typical.

---

In your example, the wait_for_deps is simply a method of the task collection, not the individual task. You don't need globals.

You need to refactor.

Gipsy

I posted a snippet of the actual code I'm using, I probably should have done that in first place. I'll try using your approach, as Refe Kettler suggested me. Thanks for your help. If after seeing the actual code you have more suggestion, I'd love to hear them. – Shipp

Answer 2

I don't suppose that there's anything wrong with this, but I don't really see how this would be sensible. Why would you need to keep a global variable (in the class, of all places) that holds references to all the instances? The client could just as easily implement this himself if he just kept a list of his instances. All in all, it seems a little hackish and unnecessary, so I'd recommend that you don't do it.

If you're more specific about what you're trying to do, perhaps we can find a better solution.

Shoa

"seems a little hackish and unnecessary". I think you can drop "a little" from that. – Gipsy

I posted a snippet from the code I'm using, maybe now it is more clear what I'm trying to do. – Shipp

@Shipp after seeing the code, it seems like you'd be best served by what S. Lott wrote as an example: a special class for the task queue – Shoa

Answer 3

This is NOT cohesive, as well as not very functional, you want to strive to get your objects as far from the 'data-bucket' mindset as possible. The static object collection is not going to really gain you anything, you need to think WHY do you need all the objects in the collection and think about creating a second class whose responsibility is to manage and be queried for all the Foobars in the system.

Mongol

Answer 4

Why would you want to do this?

There are several problems with this code. The first is that you have to take care of deleting instances -- there will always be a reference to each Foobar instance left in Foobar.foobars, so the garbage collector will never garbage collect them. The second problem is that it won't work with copy and pickle.

But apart from the technical problems, it feels like a wrong design. The purpose of object instances is hiding state, and you make them see each other.

Windrow

Answer 5

From a OOP point of view there's nothing wrong with it. A class is an instance of a metaclass, and any instance can hold any kind of data in it.

However, from an efficiency point of view, if you don't eventualy clean up the foobars dict on a long running Python program, you are having potential memory leak.

Gilstrap

Answer 6

No one has mentioned the potential problem this might have if you later derive a subclass from Foobar which could happen if the base class __init__() function is called from the derived class's __init__(). Specifically whether you want all the subclass instances to be sored in the same place as those of the base class -- which of course depend on why you're doing this.

It's a solvable problem but something to consider, and perhaps to code for, up front in the base class.

Brucine

Answer 7

I needed multiple Jinja environments in an app engine application:

class JinjaEnv(object):
    """ Jinja environment / loader instance per env_name """

    _env_lock = threading.Lock()
    with _env_lock:
        _jinja_envs = dict()                                # instances of this class

    def __init__(self, env_name):

        self.jinja_loader = .....                           # init jinja loader
        self.client_cache = memcache.Client()
        self.jinja_bcc = MemcachedBytecodeCache(self.client_cache, prefix='jinja2/bcc_%s/' % env_name, timeout=3600)
        self.jinja_env = self.jinja_loader(self.jinja_bcc, env_name)

    @classmethod
    def get_env(cls, env_name):

        with cls._env_lock:
            if env_name not in cls._jinja_envs:
                cls._jinja_envs[env_name] = JinjaEnv(env_name)   # new env
            return cls._jinja_envs[env_name].jinja_env

    @classmethod
    def flush_env(cls, env_name):

        with cls._env_lock:
            if env_name not in cls._jinja_envs:
                self = cls._jinja_envs[env_name] = JinjaEnv(env_name)   # new env
            else:
                self = cls._jinja_envs[env_name]
                self.client_cache.flush_all()
                self.jinja_env = self.jinja_loader(self.jinja_bcc, env_name)
            return self.jinja_env

Used like:

template = JinjaEnv.get_env('example_env').get_template('example_template')

Concourse