Weird reference passing in class construction

Question

I'm a web dev (game dev as a hobby), and I've seen myself use the following paradigm several times. (Both in developing server architecture and with video game dev work.) It seems really ugly, but I don't know a work around. I'll give an example in game dev, because it's where I recently noticed it. This is an RPG that I've been working on. Every time a battle is launched, the CombatEngine creates two parties of Combatants. Every Combatant sets up an ArtificialIntelligence object that's associated with the given Combatant, which is in charge of dictating moves for players which don't receive an explicit command:

public class Combatant {

    ArtificialIntelligence ai = null;

    public Combatant()
    {
        // Set other fields here.

        this.ai = new ArtificialIntelligence(this);
    }

}

Here's what I don't like: the inner field (ArtificialIntelligence) takes a Combatant during construction, because it needs some of the Combatant fields in order to dictate appropriate actions. So, for convenience, I keep a reference to the combatant passed in as an arg to the ArtificialIntelligence object, but that object contains a reference to the ai object itself! It creates this weird recursion, but I don't know how to work around it. The AI object needs a good deal of the fields that are specific to combatant, so that's why I passed in the entire object, but I don't like how the object then contains the reference to the ai field which is contained in the overlying combatant field, which is contained in the overlying ai class. Is this bad practice, or am I simply over thinking it?

Answer 1

Although there is no "design" problem here - it's just a reference you're passing - one important consideration is that you should initialize all of your fields prior to passing this to the other class. Otherwise, the other class will have access to this in a possibly inconsistent state. This is sometimes called letting this "escape" from the constructor.

Don't do this...

public class BadCombatant {

    ArtificialIntelligence ai = null;
    String someField;

    public BadCombatant() {
        this.ai = new ArtificialIntelligence(this);
        // Don't do this - ArtificialIntelligence constructor saw someField as null
        someField = "something"; 
    }

Answer 2

I would definitely avoid the cyclic dependency. In comes single responsibility principle to the rescue. You can eliminate the need to have a reference to an Artificialntelligence in Combatant by letting ArtificialIntelligence operate on a Combatant. Move all the code from Combatant that depends on the ArtificialIntelligence to ArtificialIntelligence instead. The CombatEngine will do the following :

1. Create an Independent Combatant instance that has nothing to do with Artificialntelligence.

2. Create the appropriate instance of Artificalintelligence and pass it the previously created Combatant.

Alternately, you can create a new class called CombatController that is passed a Combatant and an ArtificialIntelligence. The CombatEngine will do the following :

1. Create a Combatant with no dependencies on any other class

2. Create an Artificialntelligence with no dependency on any other class

3. Create a CombatController and pass it the Combatant and ArtificialIntelligence objects to use. The CombatController should expose methods for controlling the Combatant as well as handle AI behavior.

Regardless of which of the above approach you use, you eliminate the cyclic dependency that's bothering you.

I am sorry that I cannot provide a code example as I am typing this answer from my mobile phone and formatting is a pain.