9

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?

Sal
  • 3,179
  • 7
  • 31
  • 41
  • 6
    Are you getting a stackoverflow error? I doubt it, and if not, there's no recursion going on here, just reference passing. I think you've a non-issue here. – Hovercraft Full Of Eels Sep 19 '12 at 03:15
  • 1
    Woops, I'm a mathematician by trade, so some of my terminology is lacking. You're right, this is just reference passing. Would that still be considered a non issue? Isn't it bad practice to nest references in this form? And to answer your question, I'm not getting any errors. I just thought it looked bad, and wanted to get opinions. – Sal Sep 19 '12 at 03:16
  • Some references on circular dependencies: https://stackoverflow.com/a/37445480/1371329 – jaco0646 Oct 23 '18 at 18:05

2 Answers2

9

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"; 
    }
Bohemian
  • 412,405
  • 93
  • 575
  • 722
  • 1
    +1 Some IDE's will warn about a "leaking this" in constructors. – Paul Bellora Sep 19 '12 at 03:19
  • 1
    Well to be clear, `ArtificialIntelligence`'s *constructor* (and any methods it calls) will see `someField` as `null`. – Paul Bellora Sep 19 '12 at 03:24
  • 2
    I don't think it's a good idea to keep this cyclic dependency. See my answer! There is no need for the two classes to partially depend on one another when this dependency can be moved to one of the two classes or a completely new class. – Chetan Kinger Sep 19 '12 at 07:50
  • 3
    I also strongly disagree about this not being a design problem. As explained by bot's answer, this is a cyclic dependency, which is a design smell IMHO. See e.g. this answer: [Are circular class dependencies bad from a coding style point of view?](http://stackoverflow.com/questions/1356304/are-circular-class-dependencies-bad-from-a-coding-style-point-of-view) – sleske Sep 19 '12 at 15:10
5

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.

Chetan Kinger
  • 15,069
  • 6
  • 45
  • 82
  • +1 There are of course even more options; you could for example extract the field from Combatant which ArtificialIntelligence needs to a new class, say CombatantState. CombatantState would be a field in Combatant, and ArtificialIntelligence could accept a CombatantState. The best solution depends on the need of your code. – sleske Sep 20 '12 at 07:36
  • I totally agree. What approach to use can be clear only when you have the whole picture and not just a part of the problem. Either ways, the cyclic dependency can be easily avoided! – Chetan Kinger Sep 20 '12 at 07:38