6
public class App {
    private final A a;
    private final Server server; 
    public App(){
        a = new A(this); //Bad, this is escaping before it's initialized.
    }
    @Subscribe //This event fires some time after App is finished constructing.
    public void registerStuff(RegisterEvent event){
        server = event.getServer(); //Not possible due to final field and this not being the constructor, is there such thing as a lazy final?
        a.register();
    }
}

public class A {
    private final App app;
    private final BuilderSpec spec;
    public A(App app){
        this.app = app;
        this.spec = app.getServer().builder(this.app).build();
    }
    public void register(){
        app.getServer().doStuff(this.app, this.spec);
    }
}

I've read a little about what "this" escaping is and realize that the previous code is bad, as I have little idea what the external processes are doing with the this reference, so it shouldn't be passed outside the constructor until it is constructed.

However, due to the final fields in both App and A, I really don't see how I can initialize this afterwards, or lazily. Is making the fields final less important then the this escaping? I'm not sure.

Ryan Leach
  • 4,262
  • 5
  • 34
  • 71
  • 1
    It's isn't automatically bad, and what you have is fine. You just need to be careful that your object is in a consistent state before you pass `this` out of the constructor. It is to be avoided, yes, but if you have a circular dependency (as you do) and what both items to be `final` then there is no alternative. – Boris the Spider Jun 12 '15 at 15:08
  • Having such a circular dependency itself is not very good i guess... no?? – Codebender Jun 12 '15 at 15:11
  • 1
    @AbishekManoharan circular dependencies are perfectly natural. They occur all the time completely organically and are certainly part of normal OO modelling. – Boris the Spider Jun 12 '15 at 15:12
  • 2
    @BoristheSpider - this case does not look "organic". OP has so many concepts tangled together; a redesign with cleaner dependencies is needed. – ZhongYu Jun 12 '15 at 15:21
  • I tend to have that problem with classes that are designed to bootstrap everything else. – Ryan Leach Jun 12 '15 at 15:23

4 Answers4

2

Constructor dependencies can become a problem if they get cycled. Most of the IoC containers operate with post-construct methods to avoid problems with object instantiation.

Applying the principal to your code, it would look like

class App {
    private A a;
    public App() { 
      // not necessary to have, just to show that we have no logic inside 
    }
    protected void init() {  // choose access modifiers
        // init something in App;
        a = new A(this);  // safe to pass this now.           
    }
}

class A {
   private final App app;

   public A(App app) {
      this.app = app;
      // do whatever you need
      // if logic affects other components, create init() method here
   }      
}

Yes, A reference in App is not final. But this design is more natural and less error-prone.

AdamSkywalker
  • 11,408
  • 3
  • 38
  • 76
  • What would the code look like in the P.S. case? It feels better that A would have a final reference to App and be reinstantiated if needed rather then have a possibly changing App reference that could de-sync with A's state. – Ryan Leach Jun 12 '15 at 16:07
  • @RyanTheLeach yes, I feel that final App is better, I'll edit the code. – AdamSkywalker Jun 12 '15 at 16:09
  • init() is starting to look very similar to what would get called from the @Subscribe annotated EventHandlers in order to register/initialize stuff. Is there any difference to having this logic in the constructor to the init method? It just feels like any problem I had previously is now just transferred to whether the object is finishing initializing instead of if the object has finished constructing? – Ryan Leach Jun 12 '15 at 16:16
  • @RyanTheLeach Main difference is that you'll never cycle your dependencies, because first objects are created and only then they are initialized. If you are afraid, that RegisterEvent will be fired before A finishes initialization, you can just add some synchronization logic. – AdamSkywalker Jun 12 '15 at 16:27
  • You have a= new A(); and a.init(this) for A to have a final App, shouldn't it get passed App in the constructor? – Ryan Leach Jun 12 '15 at 16:33
  • @RyanTheLeach yes, it's a typo. – AdamSkywalker Jun 12 '15 at 16:34
1

As @Boris_the_Spider mentioned, passing this in the constructor is not necessarily a bad thing if you make sure your class is in a consistent state (see Passing “this” in java constructor). However, be careful of extending App in the future because in this case things can go very wrong (see Don't publish the "this" reference during construction)

However, another valid option (as you mentioned) would be to remove final from A. In this case, I would recommend declaring a getter method. Because A is private, it will only be accessed by class App, and so you just need to make sure App uses the getA() method whenever you need access to A:

public class App {
    private A a;
    private final Server server; 
    public App(){}

    // Define getter
    private A getA(){
        return a == null ? new A(this) : a;
    }

    @Subscribe
    public void registerStuff(RegisterEvent event){
        server = event.getServer();

        // Use getter
        getA().register();
    }
}
Community
  • 1
  • 1
bcorso
  • 45,608
  • 10
  • 63
  • 75
  • (-1 not mine) it appears that `registerStuff` is triggered by `new A()` :) the serious problem is that a mysterious `Server` object is needed during initialization of `A` and `a`, yet it can only come out of thin air. – ZhongYu Jun 12 '15 at 15:53
  • @bayou.io is that true? It looks like `A`s only method, `register()` is called inside `registerStuff` – bcorso Jun 12 '15 at 15:59
  • Sorry I should have provided some context for the RegisterEvent. RegisterEvent occurs sometime after App has been constructed, and not triggered while the constructor is not finished. – Ryan Leach Jun 12 '15 at 16:01
  • @RyanTheLeach - then how you get `app.getServer()` in `A()`? – ZhongYu Jun 12 '15 at 16:02
  • The server is passed in during the registration event, I wish I could make it final in app, as once app is initialized it should never change, however I face the problem that if I make it final I am unable to set it in registerStuff – Ryan Leach Jun 12 '15 at 16:04
  • @RyanTheLeach I think it's perfectly fine to make `A` non-final because it is `private` to the `App` class, so you can be sure that no one else can change it's value. Also, in OOP providing a getter method only (no setter) is the typical way to make something `final`ish, but has the benefit of lazy loading. In addition, while using `this` in your constructor might work out in your current case, if you look through the link I posted you can see how things can go very wrong if you ever decide to extend from `App` in the future. – bcorso Jun 12 '15 at 16:08
  • @RyanTheLeach - and `app.getServer()` in `A()` is called before registration? Don't blame final - make all fields non-final, and see what happens. – ZhongYu Jun 12 '15 at 16:11
  • I could even make App final as it isn't designed to be extended at all, if I were to have an App that could be extended I guess I could make them both inherit from an "AbstractApp" – Ryan Leach Jun 12 '15 at 16:19
  • @bayou.io that is one of my current issues I am having, trying to work out how to do it neatly. I think I might just have a stronger aversion to non final members then others, or not quite understand when they should be used yet. – Ryan Leach Jun 12 '15 at 16:21
1

Your code does not compile since you don't initialize the Server server variable which is final. Anyway you can pass a Provider which has the job of providing the App to your A when it becomes available (fully initialized). Your code can look like this:

public App(){
   appProvider = new Provider<App>()
    {
        private volatile App app;

        // setter here

        @Override
        public String get()
        {
            return app;
        }
    };
    a = new A(appProvider);

}

@Subscribe
public void registerStuff(RegisterEvent event){
    // set server to your App
    appProvider.set(this);
}

then you can check in A whether the provider's get returns null or not. I believe that the javax.inject package contains an appropriate Provider interface for you.

The registerStuff method looks good for providing the App since at that point it is fully initialized. This also answers your question about lazy initialization.

Adam Arold
  • 29,285
  • 22
  • 112
  • 207
  • Yes I am aware that the current code in the question doesn't compile, But it does show my problem quite clearly. – Ryan Leach Jun 12 '15 at 15:48
  • `A` doesn't just use `Server`, it needs `App` for the builder: `this.spec = app.getServer().builder(this.app).build();` – bcorso Jun 12 '15 at 15:49
  • I see. You can write a `Provider` for the `App` itself and provide your `App` when you have the `Server` ready as well. – Adam Arold Jun 12 '15 at 15:51
1

Do you even need an App class? A only appears to need a Server. Just move registerStuff() to from App to A and only use A.

public class A{



@Subscribe
public void registerStuff(RegisterEvent event){
    Server server = event.getServer();
    BuilderSpec spec = server.builder(this) // not sure what `builder()` needs but might only be A ?
                              .build();

    server.doStuff(this, spec);
}
 }
dkatzel
  • 31,188
  • 3
  • 63
  • 67
  • Absolutely I need an "App" class, the App is the entry point for the entire plugin. It gets constructed and is then responsible for either receiving events, or registering classes that would receive events instead. – Ryan Leach Jun 12 '15 at 15:50
  • That's what I thought but this line kills it: `this.spec = app.getServer().builder(this.app).build()` – Adam Arold Jun 12 '15 at 15:51
  • Does `App` have other fields? – dkatzel Jun 12 '15 at 15:51
  • @AdamArold not necessarily it depends on what that build method does which is why I put the comment there. – dkatzel Jun 12 '15 at 15:52
  • App has other fields, It is actually the "main" plugin class for a plugin for SpongeAPI to interact with a minecraft server. I'm not sure what it's single responsibility would be in a SOLID sense, as it seems to be a sort of genesis class to spawn many "manager" classes for different aspects of the plugin. – Ryan Leach Jun 12 '15 at 16:02