Modifying state of other objects in a constructor: design no-no?

Question

I'm refactoring some code and found this (simplified of course, but general idea):

class Variable:
    def __init__(self):
        self.__constraints = []

    def addConstraint(self, c):
        self.__constraints.append(c)

class Constraint:
    def __init__(self, variables):
        for v in variables:
            v.addConstraint(self)

The fact that the constructor of Constraint modifies other object's states instead of its own smells a little funky to me. What do other people think - is this OK, or is it a prime candidate for refactoring?

Edit: My concern is not the parent/child relationship, but that it is linked up inside the constructor rather than in a separate method.

Answer 1

I agree with you. That's backwards. There may be some good reason for why, but it's unclear programming and it likely to bite someone if the foot sooner or later.

Answer 2

I see it as a self registration pattern. "Hello I'm new here, please allow me to join."

I might prefer to have a differently named method so that the purpose is more clear, but I do actually quite like the approach.

Answer 3

This is common usage when you have two objects that are closely related (i.e. where only one of them alone doesn't make sense). Most common case: Parent child relations. When you add a child to a parent (i.e. parent.children.append(child)), you often also update the child.parent pointer.

Answer 4

I personally am not necessarily opposed to this, but...

I would choose one usage pattern, and stick with it. In your case, since Variable already has a clean addConstraint method, my preference would be to use it.

Otherwise, you'll need to add good checking to prevent the user from constructing a Constraint, and then adding it to the Variable class (thereby adding it twice).

That being said, with something like a Constraint, though, I would probably not do this. A Constraint seems like a conceptually independent entity from a Variable. I see no logical reason the same constraint couldn't be added to two separate variables. I would just make it so you construct your constraint, then add them manually, specifically for this reason.

Answer 5

I entirely concur with @djna's answer that the specific use case is perfectly legit -- here, it's an example of an object needing to en-register itself with a specified set of registries "at birth".

A very sharp and extremely common subcase of that would be an observer object that exists strictly for the purpose of observing a given observable -- perfectly fine to pass the observable to the observer's initializer, and exactly the right way to ensure the class invariant "instances of this observer class are at all times connected to exactly one observable", which would be not established "at birth" if the registration was carried out only after the completion of initialization.

Other similar cases include for example a widget object that must at all time exist within a container window: it would somewhat weird to implement it otherwise than having the widget take the parent as an initializer argument and tell the parent "hi, I'm your new child!".

At least in those 1-many cases you could imagine forcing the parent or observable to have a method that both creates and enregisters the new object. In a many-many case like this one, the somewhat inside-out nature of that approach gets revealed -- since the constraint must be registered with multiple variables, it would be "against the grain" to ask any specific one of them to create the constraint. The code you supply on the other hand is perfectly natural.

Only for cases that cannot reasonably be framed as the new object "enregistering itself" would I feel some doubt (there are a few other legit ones, such as objects creating and enregistering other auxiliary ones at birth, but they're nowhere near as common).