1

Is it a good practice to create a class attribute (list) that contains all instances of a class?

Does it have bad sideffects that i should think about?

class Car:
    cars = []
    def __init__(self, color):
        self.color = color
        __class__.cars.append(self)

    def delete(self):
        __class__.cars.remove(self)

    @classmethod
    def by_color(cls, color):
        for car in cls.cars:
            if car.color == color:
                return car

    def __repr__(self):
        return str(self)

    def __str__(self):
        return '{} Car'.format(self.color)


car1 = Car('green')
car2 = Car('blue')
car3 = Car('red')

print(Car.cars)  # [green Car, blue Car, red Car]

car3.delete()
print(Car.cars)  # [green Car, blue Car]
Frank
  • 1,959
  • 12
  • 27
  • 3
    Take note that ``car3.delete()`` **does not** delete ``car3``. You can still use the ``car3`` name afterwards, and it is a fully functional instance of the class. Consider "it is likely to be done incorrectly" a bad sideffect of such a pattern. – MisterMiyagi Oct 15 '19 at 13:57
  • 2
    You can definitely *do* it but why? There's no need to explicitly link each car to the Car class when you know by construction that each car is intrinsically part of the Car class. If you wanted to keep track of cars maybe have a "Garage" class or something. Then it would make sense for a Garage to have cars. and you could do something like Garage.add(car). – Brian Oct 15 '19 at 14:00
  • 2
    @BrianJoseph There are definitely use cases for tracking object instances. A `Garage` class would be superfluous if all OP needed was a `list` within the main scope. – r.ook Oct 15 '19 at 14:06
  • 1
    The [Flyweight pattern](https://en.wikipedia.org/wiki/Flyweight_pattern) is often implemented by keeping a collection of immutable instances, so there are definitely reasons to do this. That said, I think 99% of the time you'd be better off keeping a list in whatever context you're creating the instances in, rather than keeping the list in the class. – Kevin Oct 15 '19 at 14:13
  • 1
    @Krrr, I suppose so. I don't fully understand OPs implementation so I can't say whether or not there's a better way to do something. Either way, here's how I'd go about implementing this in a project, OP: https://stackoverflow.com/a/328882/9307263. Note that this is answer is very old and the weakref module has changed. – Brian Oct 15 '19 at 14:15
  • It was only for information if such practice would work. I never needed to do it this way but i thought if it could make some things easier. Thank you all for all responses! – Frank Oct 15 '19 at 14:23

1 Answers1

3

Is it a good practice to create a class attribute (list) that contains all instances of a class?

No. While such a pattern can be useful, consider it to be unexpected for most people. Use it only if there is a clear benefit and document it rigorously.

Does it have bad sideffects that i should think about?

While not necessarily bad, various side-effects will not be obvious to users of the class.

  • It practically removes automatic garbage collection for instances. At the very least, Car.cars should use weak references to allow objects to be collected eventually.

  • It enforces global scope for instances. If two threads create cars, they share all instances. It is not (easily) possible to retrofit thread or task locality.

  • Behaviour under subclassing is not obvious. If Truck extends Car, does Car.cars contain trucks? If Truck.cars exists, is its content duplicated in Car.cars as well?

  • Using a list of instances will be very slow for many instances. Both Car.delete and Car.by_color grow slower with the number of instances. Consider using a (weak) set or dictionary.

MisterMiyagi
  • 44,374
  • 10
  • 104
  • 119