OO design and circular dependencies

Question

I am currently struggling with a circular dependency problem when designing my classes.

Ever since I read about the Anemic Domain Model (something I was doing all the time), I have really been trying to get away from creating domain objects that were just "buckets of getters and setters" and return to my OO roots.

However, the problem below is one that I come across a lot and I'm not sure how I should solve it.

Say we have a Team class, that has many Players. It doesn't matter what sport this is :) A team can add and remove players, in much the same way a player can leave a team and join another.

So we have the team, which has a list of players:

public class Team {

    private List<Player> players;

    // snip.

    public void removePlayer(Player player) {
        players.remove(player);
        // Do other admin work when a player leaves
    }
}

Then we have the Player, which has a reference to the Team:

public class Player {
    private Team team;

    public void leaveTeam() {
        team = null;
        // Do some more player stuff...
    }
}

One can assume that both methods (remove and leave) have domain-specific logic that needs to be run whenever a team removes a player and a player leaves a team. Therefore, my first thought is that when a Team kicks a player, removePlayer(...) should also call the player.leaveTeam() method...

But then what if the Player is driving the departure - should the leaveTeam() method call team.removePlayer(this)? Not without creating an infinite loop!

In the past, I'd have just made these objects "dumb" POJOs and had a service layer do the work. But even now I'm still left with that problem: to avoid circular dependencies, the service layer still has link it all together - i.e.

public class SomeService {

    public void leave(Player player, Team team) {

        team.removePlayer(player);
        player.leaveTeam();

    }

}

Am I over complicating this? Perhaps I'm missing some obvious design flaw. Any feedback would be greatly appreciated.

---

Thanks all for the responses. I'm accepting Grodriguez's solution as it is the most obvious (can't believe it didn't occur to me) and easy to implement. However, DecaniBass does make a good point. In the situation I was describing, it is possible for a player to leave a team (and be aware of whether he is in a team or not) as well as the team driving the removal. But I agree with your point and I don't like the idea that there's two "entry points" into this process. Thanks again.

Tractate

Answer 1

You can break the circular dependency by adding guards to check if the team still has the player / the player is still in the team. For example:

In class Team:

public void removePlayer(Player player) {
    if (players.contains(player))
    {
        players.remove(player);
        player.leaveTeam();
        // Do other admin work when a player leaves
    }
}

In class Player:

public void leaveTeam() {
    if (team != null)
    {
        team.removePlayer(this);
        team = null;
        // Do some more player stuff..
    }
}

Headachy

May be its just me, but I like to use if...else as sparingly as possible. I have noticed it makes code a little less maintainable – Rhombus

players.remove() will return true if the collection was changed; no need to do the .contains(). – Guinea

@KarlP: I know, but I thought the explicit check would make the logic more clear. – Headachy

The closing parenthesis of the if statement in the first code example is missing. – Doorstep

What if a single Player can enter a Team multiple times? It probably doesn't make sense for sport teams, but I can imagine situations where this could happen. – Macrobiotics

@Lie Ryan: As you say didn't seem to make much sense in this case, but of course it is something to bear in mind for other cases. – Headachy

@Lie Ryan: Thats a ManyToMany relation. One way to model that is to figure out a new logical entity with one-to-many relations from both the players and the teams. In the current example it might be relevant for storing historical data for all the teams a player has played in. In this case it is probably a "Contract" :-) – Guinea

Answer 2

Ben,

I would start by asking if a player can (logically, legally) remove himself from the team. I would say the player object doesn't know what team he's in (!), he is part of a team. So, delete Player#leaveTeam() and make all team changes occur through the Team#removePlayer() method.

In the case where you only have a player and need to remove it from its team, then you could have a static lookup method on Team public static Team findTeam( Player player ) ...

I know this is less satisfying and natural than a Player#leaveTeam() method, but in my experience you can still have a meaningful domain model.

2 way references (Parent -> Child and Child-> Parent) are often fraught with other things, say Garbage Collection, maintaining the "referential integrity", etc.

Design is a compromise!

Jerald

Answer 3

Idea is to do domain-related stuff in different methods which do not call each other but does domain related stuff for their own object, i.e. team's method does it for team and player's method does it for player

public class Team {

    private List<Player> players;

    public void removePlayer(Player player) {
        removePlayerFromTeam(player);
        player.removeFromTeam();
    }
    public void removePlayerFromTeam(Player player) {
        players.remove(player);
        //domain stuff
    }
}

public class Player {
    private Team team;

    public void removeFromTeam() {
         team = null;
        //domain stuff
    }
    public void leaveTeam() {
        team.removePlayerFromTeam(this);
        removeFromTeam();
    }

}

Rhombus

The leaveTeam() method would throw a NPE as you call team.removePlayerFromTeam() after setting team = null. – Headachy

Also in this solution, calling player.leaveTeam() does not actually remove the player from the player's list in the team object. Likewise, calling team.removePlayer() will not set the team var to null in the player object. – Headachy

In this design the methods containing domain-specific code should be package-private and not public, I think. But it's definitely the route I'd take. – Revoice

@Waldheinz: I agree, if thats a possibility, its the best way – Rhombus

Answer 4

public void removePlayer(Player player) {
    if (players.contains(player)) {
        players.remove(player);
        player.leaveTeam();
    }
}

Ditto inside leaveTeam.

Juxtapose