Infinite recursion composition class

Question

I have an infinite recursion in my program where I have a field in a class, that has the same class in its field. They are singletons, but this is not what causes them not to construct. By the way I wrote the program I actually cannot remove the phases array.

abstract class Phase{
  protected String phaseName;
  protected char[] keys;
  protected String[] commands;
  protected Phase[] phases;
  protected StringBuilder pattern;

}

class RemotePhase extends Phase{
  private static RemotePhase remotePhase;

  protected RemotePhase(){
    phaseName="Remote.";
    commands=new String[]{"Lock/unlock windows", "Toggle door", "Select dog menu"};
    setPattern();

    //Just below here starts an infinite loop
    phases=new Phase[]{FixWindows.getFixWindows(), ToggleDoor.getToggleDoor(), SelectDogPhase.getSelectDogPhase()};

  }

  public static RemotePhase getRemotePhase(){
    if(remotePhase==null){
      remotePhase=new RemotePhase();
    }
    return remotePhase;
  }
}

final class FixWindows extends Phase{
  private static FixWindows windows;
  private RemotePhase remotePhase;

  private FixWindows(){

    //execution keeps coming here as FixWindows object is never constructed
    remotePhase=RemotePhase.getRemotePhase();

  }

  public static FixWindows getFixWindows(){
    if(windows==null){
      windows=new FixWindows();
    }
    return windows;
  }
}

I have tried making RemotePhase a static class and FixWindows use it for its members, but I ran into errors trying to override the non static methods of the abstract class, and trying to call them from FixWindows in nonstatic context. I prefer to not make it static though, because I would have to make an additional class just to refer to RemotePhase.

Any ways to make this work though. Thanks

Change `FixWindows` constructor and pass `RemotePhase` (this) to build that in `getFixWindows` or just set `remotePhase` in `getFixWindows`. Try to avoid calling constructor in other constructor. Your static methods are good for setting such these stuff — Ebrahim Pasbani, Aug 07 '16 at 01:50
See [this answer](http://stackoverflow.com/a/16106598/256196) for a simple way to implement a singleton — Bohemian, Aug 07 '16 at 02:27
thank you, reminds me that I saw this strategy in an article — Danny P., Aug 07 '16 at 03:05

Dici · Accepted Answer · 2016-08-07T02:13:15.727

4

Why would you need to store a reference to a singleton while you can always access it by the static getter ? This would enable lazy access to RemotePhase from FixWindows, and fix your circular dependency. Therefore, the cleanest fix is simply to not call the getter inside the constuctor of FixWindows.

final class FixWindows extends Phase{
  private static FixWindows windows;

  private FixWindows(){
      // does nothing but preventing external classes to instantiate it
  }

  public static synchronized FixWindows getFixWindows(){
    if(windows==null){
      windows=new FixWindows();
    }
    return windows;
  }

  public void methodThatRequiresTheRemotePhase(){
    doSomeStuff(RemotePhase.getRemotePhase());
  }
}

By the way, I should warn you that your code is not thread-safe. Your getters should be synchronized.

edited Aug 07 '16 at 02:13

answered Aug 07 '16 at 01:56

Dici

25,226
7
41
82

@DannyP. I edited my post with an example and a warning on thread-safety – Dici Aug 07 '16 at 02:09
Okay, I can understand better now... While I was writing the code to set up the automatic execution of the phases, I ended up having to store an array of phases for each phase. – Danny P. Aug 07 '16 at 02:12
I don't know what you are doing but I would recommend to think twice about using this many singletons. Usually you would use singletons mainly to avoid loading or serializing very large resources multiple times (or thread pools, stuff like that), but you don't seem to be doing that so I wonder if such complexity is useful – Dici Aug 07 '16 at 02:15
Okay I am not using it for that, I just have a map returned by another map, that is set by looping through these arrays: names, keys, and phases. I wanted to use only one object for the phases and other classes and keep track. – Danny P. Aug 07 '16 at 02:25

score 1 · Answer 2 · edited May 23 '17 at 12:08

This is an answer to how to break an initialization loop. Answer by @Dici is however better, since you shouldn't need to store singleton references to RemotePhase in the first place.

Your problem is that the singleton-initializers are co-dependent.

One way to break that, is to assign the static singleton field of RemotePhase before the RemotePhase in initialization is complete. This will then ensure that when initialization progresses, and the FixWindows singleton object is constructed, it can find the (partially) initialized RemotePhase singleton object.

So, chance code like this:

private RemotePhase() {
    phaseName = "Remote.";
    commands = new String[] { "Lock/unlock windows",
                              "Toggle door",
                              "Select dog menu" };
    setPattern();
}

private void init() {
    phases = new Phase[] { FixWindows.getFixWindows(),
                           ToggleDoor.getToggleDoor(),
                           SelectDogPhase.getSelectDogPhase() };
}

public static RemotePhase getRemotePhase() {
    if (remotePhase == null) {
        remotePhase = new RemotePhase(); // assigns partially initialized object
        remotePhase.init();              // completes initialization
    }
    return remotePhase;
}

Ebrahim Pasbani · Answer 3 · 2016-08-07T02:01:02.307

You should avoid calling constructor in another ones. You can use setters instead.

I show this idea for FixWindows, you can use the same for other subclasses

abstract class Phase{
  protected String phaseName;
  protected char[] keys;
  protected String[] commands;
  protected Phase[] phases;
  protected StringBuilder pattern;

}

class RemotePhase extends Phase{
  private static RemotePhase remotePhase;

  protected RemotePhase(){
    phaseName="Remote.";
    commands=new String[]{"Lock/unlock windows", "Toggle door", "Select dog menu"};
    setPattern();

    //Just below here starts an infinite loop
    phases=new Phase[]{FixWindows.getFixWindows(this), ToggleDoor.getToggleDoor(), SelectDogPhase.getSelectDogPhase()};

  }

  public static RemotePhase getRemotePhase(){
    if(remotePhase==null){
      remotePhase=new RemotePhase();
    }
    return remotePhase;
  }
}

final class FixWindows extends Phase{
  private static FixWindows windows;
  private RemotePhase remotePhase;

  private FixWindows(){

    //execution keeps coming here as FixWindows object is never constructed
    //remotePhase=RemotePhase.getRemotePhase(); //shoud be deleted

  }

  public static FixWindows getFixWindows(remotePhase){
    if(windows==null){
      windows=new FixWindows();
      windows.setRemotePahse(remotePhase);
    }
    return windows;
  }
}

Infinite recursion composition class

3 Answers3