How to fix a pair of tightly coupled classes that store references to each other?

Question

I have the following situation:

    class User
    {
        public Thing curThing;
    }
    //each thing can only belong to one user at a time
    //And vice versa
    class Thing
    {
        public User curUser;
    }

The curThing and curUser references need to match for a given pair. In some places in the code I have a reference to a thing and I need detach the curUser and set it to a new one. In other places I have a User reference and need to do the same.

This is quite messy. Moreover, it is quite hard to establish a given order for the operation of these two. Whenever I modify one of them, I often need to call a corresponding function in the other class. If I am not careful about which functions call methods in the other class, I can end up with a stack over flow.

Is there a name for this particular anti-pattern and is there better way to do this?

Answer 1

Whenever you have tightly coupled objects with circular dependencies, you have one of three cases (in an order of likelihood that I just pulled out of thin air, but sounds plausible):

1. You are missing an object, and the two objects are actually three objects.
2. The two objects are actually one object.
3. The business domain you are modeling is indeed circular, and your model is perfectly correct.

In your case, it is very hard to judge which of the three is the correct one, because there is not enough context in your question, but I think we are looking at #1, and the missing object is an ownership relation:

class User {}

class Thing {}

record Ownership(User user, Thing thing);

You'd also need some sort of ledger to keep track of the current ownership. This would have the additional advantage that User, Thing, and Ownership could all be immutable because the relationship between the three of them is purely maintained by what is the current version of the ledger. It also moves the problem of how to atomically update the relationship to a single place because only the ledger has to be atomically updated.

Alternatively, don't have a separate ownership object for each individual relation but only a global ownership registry. This would essentially be a bi-directional map. Again, concurrency problems can be isolated to a single place, by only requiring atomicity of the update and read operations on the map.

If you think about "the real world" (whatever that is): a thing usually does not know who it is owned by. An owner typically knows what things they own. But, sometimes things are more complicated, and we need some sort of third-party that keeps track of who owns what. (Think of land ownership, for example.)

Answer 2

The general relationship you have between your classes (0..1 User to 0..1 Thing) is a perfectly valid kind of relationship. It's less common, but valid in the right situation, which this seems to be.

However, you need to be very careful here. Your current setup creates a lot of room for contradiction. Part of this is because you store this information twice. Another part of the issue is that you are treating User and Thing as equals, where either of them is allowed to direct the other. That's causing you a lot of grief because there's no clear hierarchy.

These issues are closely tied together. If you stop treating them as equals and instead assign one of them to be the owner of the other; then you can resolve a lot of this ambiguity and keep your code more elegant.

When you store the relationship twice, you start running the risk of things contradicting each other. Using a simpler (somewhat silly) example:

public class Foo
{
    public string Name { get; set; }
    public int LengthOfName { get; set; }
}

These two pieces of information stem from the same source. But because we store them separately, we're able to create states which contradict one another:

var foo = new Foo() { Name = "Bob", LengthOfName = 5 };

You might say that this is clearly wrong and easily fixed, but it can get very messy:

var foo = new Foo() { Name = "Bob", LengthOfName = 3 };

// lots of logic

foo.Name = "Robert";

Console.WriteLine(foo.LengthOfName); // Hey, why is this 3!?

What we should do here is ensure that these two pieces of information stem from the same root:

public class Foo
{
    public string Name { get; set; }
    public int LengthOfName => Name.Length;
}

Now, it's impossible for the name and its length to go out of sync.

This same issue applies to your case:

var user = new User();
var differentUser = new User();

var thing = new Thing();
var differentThing = new Thing();

user.Thing = thing;
thing.User = differentUser;            // That's not symmetrical!
differentUser.Thing = differentThing;  // And neither is this!

The fact that this is possible means that your logic is allowing for more options than are allowed by your business logic.

The simple solution is to only store one of these references. You get to pick which one you want (this related to the second issue I pointed out, i.e. a lack of hierarchy), but the end result is that one of them holds a reference to the other, but not vice versa.

This means that there's no longer a contradiction. However, this also limits your navigation properties, as you can now only travel from one to the other, not the other way around.

If that's not a problem for you, great! Stop reading here and enjoy your solution. But if you do want two-way navigation, read on.

---

When you are dealing with an ORM (I'll use Entity Framework here as an example), then you can work around this issue by defining two navigational properties but only one FK:

public class User 
{
    public virtual Thing Thing { get; set; }
}

public class Thing
{
    public int UserId { get; set; }
    public virtual User User { get; set; }
}

However, if you're dealing with an in-memory collection or a data store that doesn't allow you to separate the FK from the navigational property, then your options are limited.

Either you have the dual relationship (which is an open door to bugs), a one-way navigation, or you have to develop a way for the other direction of the relationship to be resolved, e.g.:

public class User
{
    // User can directly navigate to Thing

    public Thing Thing { get; set; }
}

public class Thing
{
    // Thing has to indirectly go out and find its User

    public User User 
    {
        get
        {
            var allUsers = ....; // get a list of all users

            return allUsers.SingleOrDefault(user => user.Thing == this);
        }
    }
}

But it's not hard to see that this requires access to your complete datastore. Admittedly, when it's in-memory, that's not a huge cost, but it is a dependency that shouldn't really be there (I think this is debatable either way - whether the dependency is justified by needing to use it or not)

Similarly, you can also delegate the "find a thing's user" logic to a repository method and just skip the navigational property altogether. Or you can set your repository up so that you always return a user and their thing together, and you never have to fetch one based on the other.

You could also change the inferior object (if you've decided a hierarchy) to be a value object that is part of the other object, but then these objects cease to have their own lifetime. It's unclear whether this is suitable for your case.

As you can see, there are many ways to resolve this issue (and I don't think I've exhausted the options yet), but you need to figure out which one is the right one for your case.

Answer 3

Whenever I modify one of them, I often need to call a corresponding function in the other class. If I am not careful about which functions call methods in the other class, I can end up with a stack over flow.

When reading this, my first thought was that User and Thing really should have been one class, but that then doesn't match with moving a Thing instance between User instances.

The fact that both User and Thing know about each other is not ideal, but that is not an anti-pattern that must be resolved. Sometimes such mutual knowledge is just a fact of life.

The need to call a corresponding function in the counterpart (for something other than updating the link between instances) is a problem that needs to be resolved. This is suggestive that something has gone wrong in the Separation of Concerns between the classes and perhaps that you have split your problem into the wrong set of classes.
You should critically examine your design if you can come to a design where each method of a class (except those that manage a strict 1-to-1 relation) either

• delegates to a similarly named method in a counterpart class, or
• performs the work, which might involve calling different methods in a counterpart class

Answer 4

One way to ensure consistency of circular references such as this is to implement idempotent set/unset operations which call each other recursively unless the target condition is already satisfied. I don't speak C#, so please accept pseudocode:

class User
{
    public Thing curThing;

    function setThing(Thing aThing)
    {
        if (this.curThing == aThing)
            return;
        if (this.curThing != NULL)
            curThing.unsetUser(this);
        this.curThing = aThing;
        this.curThing.setUser(this);
    }

    function unsetThing(Thing aThing)
    {
        if (this.curThing == aThing)
        {
            this.curThing = NULL;
            aThing.unsetUser(this);
        }
    }
}

/* class Thing is completely symmetrical */

That way, it doesn't matter if a client calls user.setThing() or thing.setUser(), it will always result in the same (consistent) state.

However, the advice given in the other answers should be considered seriously - structures such as this are probably not an antipattern but possibly a code smell.

Answer 5

The name of that particular anti-pattern is procedural design.

In short, you are separating data from behavior, which arguably disqualifies this kind of design from being called object-oriented at all. More importantly it pushes the responsibility to handle these data things right to the caller, which is exactly your problem.

The way to fix this, is to hide data (i.e. make it private at don't leak it in any way) and to include behavior into the object. Not just any behavior, but business behavior. So don't do CRUD methods, do "freezeCreditCards()", "reportUsage()", "display()" or whatever the requirements need.