Passing "this" to private final members in Java

Question

Based on What is an "incompletely constructed object"? and How do JVM's implicit memory barriers behave when chaining constructors? using this in constructor is considered bad practice and should be avoided. Is same also true for initializing private final members?

For example code below, is passing this to Chapter considered bad practice? I understand if Book title also initialized same way as chapters then this issue won't show itself, issue being book.title is null in Chapter constructor.

public class Book {
    protected final String title; // = "Title" 

    private class Chapter {
        private final Book book;
        Chapter(Book b) {
            book = b; // Here book.title is null
        }
    }
    private final Chapter chapter1 = new Chapter(this); // Bad Practice or valid in some cases? 
    private final Chapter chapter2 = new Chapter(this);
    Book() {
        title = "Book: 2 Chapters";
    }
}

Answer 1

Yes, bad practice. In the sense that it is an effectively identical case as passing this around in a constructor.

During the initialization of an object, there's a rather complex rigamarole that the runtime has to go through, considering that [A] the class itself must be available first (and this involves static initialization which is also quite complicated), [B] the constructor must run, but in order to do that, the invoked constructor of the parent must run first, and [C] constructor is not all, there's also the instance initializer block!

Whenever you have a (non-static) field that is assigned a value, then there are only 2 options:

A. That value is a compile time constant, such as private int x = 5;. There are very few constants - basically, the primitives, and strings.

B. Anything else.

Constants as per 'A' are stored directly in the class file. But anything else cannot 'just' be stored directly in the class file (the block that lists fields has room to state that its value must be initialized to a specific constant from the constant pool, but System.currentTimeMillis(), or new Chapter(this) is obviously not something you can stick in the constant pool). At the class file level, this is essentially just all moved into the constructor for you. An actual method is made with a funky name (<init>, which isn't a valid identifier so it cannot possibly clash with an actual method named <init>, as that wouldn't be valid java) and that is invoked. You can write initializers yourself, using an exotic bit of java syntax: Just toss braces straight in your class definition:

class Test {
  public Test() {
    hello(30);
  }

  int foo = 5; // constant
  int bar = hello(10);
  { System.out.println("Instance initializer"); }
  int baz = hello(20);

  static int hello(int in) {
    System.out.println("Hello: " + in);
    return in;
  }

  public static void main(String[] args) { new Test(); }
}

You can compile and run the above and you get:

Hello: 10
Instance initializer
Hello: 20
Hello: 30

which shows you exactly how this works: All 'instance initializations' are run in 'lexical order' (in the order they appear in your source file), and there is no difference between private int bar = hello(10); and {hello(20);} - these are both instance initializers.

The problem with yelling 'bad practice!!!!' is that programming is just too difficult for that sort of rote application of rules. If it was that simple we'd just let the computers program themselves.

No, strict adherence to a list of bad practices just means you end up with even crappier code. You can't "rule of thumb" your way to clean code ('clean' defined as: Easy to read (which in turn means: You quickly come to an initial impression about what the code does when glancing at it, and this initial impression is not misleading), easy to test, flexible (defined as: When expectable change requests come in it is not particularly hard to process these and does not require many changes to code that interacts with this code, i.e. few to no API changes for example)).

So, you must first understand why 'it is bad practice' to pass this around in constructors. Failure to understand why means the rule is useless to you.

The reason why is - you're halfway through initialization and variables may not have a value yet. Let's see it in action!

class Test {
  final int bar = hello(this, 10);
  final int baz = hello(this, 20);
  final int foo = hello(this, 30);

  public static int hello(Test t, int v) {
    System.out.println("BAZ: " + t.baz);
    return v;
  }

  public static void main(String[] args) {
    new Test();
  }
}

Running this prints:

BAZ: 0
BAZ: 0
BAZ: 20

Which is utterly, off the wall, nuts. How in the blazes is a final variable, no less, changing value!!?!!!

And that's the biggest reason why this is a very bad idea. There are quite a few more, but this one is easy to understand and sufficient to show why you should never be doing this. This act generally falls under the 'incredibly confusing hackery' category. Which means if you must resort to such a thing, you must add a ton of comments about why, and what's going to happen, and precisely which pre- and post conditions you expect. Generally however much time you spend on writing and maintaining those comments (and remember, maintaining comments is really difficult, because you cannot unit test them!), surely that's not worth whatever shortcut you think you are achieving with doing this. But, hey, if truly it is worth it, then by all means. Hack away. Don't forget those comments.

EXERCISE FOR THE READER: If you want to learn more, I suggest you write a more extensive example which also involves static initializers (non-constant expressions that initialize static fields, as well as static { codeGoesHere(); }, just like that in a class definition, e.g. class Foo { static { System.out.println("HELLO!"); }}), and a hierarchy of types. For bonus points, have the constructor of the parent type invoke a method that isn't final, and then override that method in your subclass and have that method interact with fields. Example:

class Parent {
  Parent() {
    System.out.println(thisIsWeird());
  }

  String thisIsWeird() {
    return "Parent";
  }
}

class Child extends Parent {
  private final String field = "Simple".toLowerCase();

  @Override String thisIsWeird() {
    return field;
  }
}

Then run it, and start drawing conclusions (and then prepare to forget all these nitty gritty details and remember only the upshot, which is: Don't do this crazy stuff, it's not worth it).