Assign "this" to a private field in constructor

Question

I was looking around but I can't find a definitive answer about assigning this in a private field is an anti-pattern in Java. Consider the example below of a singleton-based pattern:

public class Foo {

    private static Foo INSTANCE;

    private Foo() {
        INSTANCE = this;
    }
}

My guess is that at the time of the declaration, this is not fully initialised so it's not safe as if any other call would use the static field it might find an instance that it's not fully initialised.

Is that correct though? Is this something that we should avoid? If yes, why? Is there any way to make sure that those calls are safe (as in that we don't use INSTANCE any further in the constructor)?

Why do you think this is something you need to do? I.e. what problem do you have that would ever require this pattern that cannot be solved better by something that respects the instance/static contexts? As for the quoted text: no. When the private `Foo` constructor gets called there is, by definition, an established `this`. This code will work, but it also makes very little sense. — Mike 'Pomax' Kamermans, Jun 19 '20 at 16:16
It's not something I "need" to do, just asking if I should avoid doing it or not. It's something I was thinking today and couldn't find out if it's safe to have something like this — Skod, Jun 19 '20 at 16:20
Okay, but that is dependent on what you need: without any explanation of why this would be code you actually wrote and used, this code makes no sense and isn't so much an anti-pattern as just "not even a pattern": you're constantly overwriting what `INSTANCE` is, so I can't think of a single actual thing you might want to do that would ever require code like this. — Mike 'Pomax' Kamermans, Jun 19 '20 at 16:23
For example: if you want to track instances built, then you'd use something like `public static Foo build(...) { Foo f = new Foo(...); Foo.instances.add(f); return f; }` with a static list-of-Foo. On the other hand, if you need a universal singleton, you'd have a `public static Foo getInstance() { if (Foo.instance == null) { Foo.instance = new Foot(); } return Foo.instance; }` with that private static Foo field (and then you probably want an `enum`, not a `class`) — Mike 'Pomax' Kamermans, Jun 19 '20 at 16:26
@Mike lol I actually liked the quote of "This code is so dumb that it's not even a pattern". Your comment about `this` keyword that is provided before the constructor starts actually answers the question. It's fine to use something like this - in terms of java code safety and syntax. But again, it's something that I was thinking today, not a real use case — Skod, Jun 19 '20 at 16:36
While it might _work_, it's most certainly not _fine_. This code effectively guarantees future bugs, and would get flagged as needing a rewrite in any real codebase =) — Mike 'Pomax' Kamermans, Jun 19 '20 at 17:53

score 6 · Answer 1 · edited Jun 25 '20 at 15:25

6

Every time you construct a new instance of this class, you are overriding the field. Really a better name than INSTANCE would be LAST_CREATED_INSTANCE.

That said, given that the field is not declared volatile, a write to that field does not have guaranteed visibility across threads. So it is more like SOMEWHAT_RECENTLY_CREATED_INSTANCE.

So yes, it's bad. I can't see any situation where the implementation as presented would be an optimal solution.

Singleton implementations in Java have been discussed at length already. There should be no real reason why you should have to devise your own, except as an exercise.

See Item 3 of Josh Bloch's Effective Java, excerpts of which are here: What is an efficient way to implement a singleton pattern in Java? (though I would advise reading the whole book)

edited Jun 25 '20 at 15:25

Basil Bourque

303,325
100
852
1,154

answered Jun 19 '20 at 16:19

Michael

41,989
11
82
128

I should have mentioned that but I just ignored thread safety on that part. I'm aware of singletons, the question above is something that I was thinking today, it may not have an actual use-case but I was more interested if java would be fine by the use of it (as it's syntactically correct) – Skod Jun 19 '20 at 16:31
@DimitrisSkoufis Can you define what you mean by "be fine with it". If it is syntactically correct, then it will compile. By my definition, I'd say that we can conclude that Java's fine with it. But given you know that, I'm guessing you have a different definition, which must have something to do with runtime behaviour? In which case, what is stopping you from testing it and observing what happens? – Michael Jun 19 '20 at 16:39
Correct; I'm more concerned about runtime issues, i.e if a child of `Foo` call its parent constructor - or something around this. – Skod Jun 29 '20 at 21:03

score 0 · Answer 2 · answered Jun 19 '20 at 16:48

This is definitely not a classic self-reference because you are using a static variable. So I will skip the classic wording for self-references.

What you are doing is that you are keeping a reference to the last created object. One use-case will be for example something like this:

public class Users {
   private static Users LAST_USER = null;
   ...
   private Users() {
       LAST_USER = this;
       ...
   }
}

So in a static variable you keep a reference to the last user and can eventually print his name on the page (X just logged in) or something like that. Sounds good as an example, right?

Will it work? Yes, it will.

But is it good? It really depends on what you are doing with those objects. If it is going to live "forever", then fine. But what about if you wish to "delete" objects (like from my example the last user logs off right after logging in)? It will fail immediately as logic and usefulness - it will create more hassle to resolve such simple cases than it will help.

So I consider it a bad idea; however you may eventually find a better example where such thing may eventually be useful. If it is, why not?

score 0 · Answer 3 · answered Jun 19 '20 at 20:39

One way to avoid leaking this in a constructor is with a static factory method. Also, as Michael has pointed out, INSTANCE should be volatile.

public class Foo {

    private static volatile Foo INSTANCE;

    private Foo() {
        // ...
    }

    private static Foo create() {
        // This should probably be synchronized for atomicity
        // but that's for another question
        Foo newInstance = new Foo();
        INSTANCE = newInstance;
        return newInstance;
    }
}

Note that this is not a singleton, nor is the original code.

Assign "this" to a private field in constructor

3 Answers3