77

In Kotlin, it warns you when calling an abstract function in a constructor, citing the following problematic code:

abstract class Base {
    var code = calculate()
    abstract fun calculate(): Int
}

class Derived(private val x: Int) : Base() {
    override fun calculate(): Int = x
}

fun main(args: Array<String>) {
    val i = Derived(42).code // Expected: 42, actual: 0
    println(i)
}

And the output makes sense because when calculate is called, x hasn't been initialized yet.

This is something I had never considered when writing java, as I have used this pattern without any issues:

class Base {

    private int area;

    Base(Room room) {
        area = extractArea(room);
    }

    abstract int extractArea(Room room);
}

class Derived_A extends Base {

    Derived_A(Room room) {
        super(room);
    }

    @Override
    public int extractArea(Room room) {
        // Extract area A from room
    }
}

class Derived_B extends Base {

    Derived_B(Room room) {
        super(room);
    }

    @Override
    public int extractArea(Room room) {
        // Extract area B from room
    }
}

And this has worked fine because the overriden extractArea functions don't rely on any uninitialized data, but they are unique to each respective derived class (hence the need to be abstract). This also works in kotlin, but it still gives the warning.

So is this poor practice in java/kotlin? If so, how can I improve it? And is it possible to implement in kotlin without being warned about using non-final functions in constructors?

A potential solution is to move the line area = extractArea() to each derived constructor, but this doesn't seem ideal since it's just repeated code that should be part of the super class.

rosghub
  • 8,924
  • 4
  • 24
  • 37
  • 1
    I've reopened the question since it asks about Kotlin, not Java. While Kotlin and Java are similar in this aspect, Kotlin instance construction is not equivalent to that in Java (at least it is expressed in different constructs). – hotkey May 07 '18 at 22:25
  • @hotkey ok, I think the accepted answer explains it pretty well. My question may have been more related to java anyway. It was the warning in kotlin that sparked the question though. – rosghub May 07 '18 at 22:35
  • @hotkey "*So is this poor practice in java/kotlin*" - Seems like they're asking for either. Their confusion comes from the Java code working (the "bug" not appearing), which works because the it doesn't actually replicate what the Kotlin code is doing. The answer in the marked duplicate definitely explains the warnings & results he's obtaining. – Vince May 07 '18 at 23:21

3 Answers3

98

The initialization order of a derived class is described in the language reference: Derived class initialization order, and the section also explains why it is a bad (and potentially dangerous) practice to use an open member in initialization logic of your class.

Basically, at the point when a super class constructor (including its property initializers and the init blocks) is executed, the derived class constructor has not yet run. But the overridden members retain their logic even when called from the super class constructor. This may lead to an overridden member that relies on some state, that is specific to the derived class, being called from the super constructor, which can lead to a bug or a runtime failure. This is also one of the cases when you can get a NullPointerException in Kotlin.

Consider this code sample:

open class Base {
    open val size: Int = 0
    init { println("size = $size") }
}

class Derived : Base() {
    val items = mutableListOf(1, 2, 3)
    override val size: Int get() = items.size
}

(runnable sample)

Here, the overridden size relies on items being properly initialized, but at the point when size is used in the super constructor, the backing field of items still holds null. Constructing an instance of Derived therefore throws an NPE.

Using the practice in question safely requires considerable effort even when you don't share the code with anyone else, and when you do, other programmers will usually expect open members to be safe to override involving the state of the derived class.


As @Bob Dagleish correctly noted, you can use lazy initialization for the code property:

val code by lazy { calculate() }

But then you need to be careful and not use code anywhere else in the base class construction logic.

Another option is to require code to be passed to the base class constructor:

abstract class Base(var code: Int) {
    abstract fun calculate(): Int
}

class Derived(private val x: Int) : Base(calculateFromX(x)) {
    override fun calculate(): Int = 
        calculateFromX(x)

    companion object {
        fun calculateFromX(x: Int) = x
    }
}

This, however, complicates the code of the derived classes in cases when the same logic is used both in overridden members and for calculating the values passed to the super constructor.

Patrick Tyler
  • 191
  • 2
  • 9
hotkey
  • 140,743
  • 39
  • 371
  • 326
  • I see. I realized it was poor practice once I understood the lint warning. Thank you for that explanation – rosghub May 07 '18 at 22:27
  • good explanation! But I still wonder what's the mechanism behind it. How would compiler know when to assagin 0 to and when to not to initiate ? How does the compiler know when creating Derived class, assigning a value to open property needs to wait until derived class constructor be run? Just curious. – BobTheCat Feb 10 '22 at 10:05
10

It is definitely poor practice because you are invoking calculate() on a partially constructed object. This suggests that your class has multiple phases of initialization.

If the result of calculation() is used to initialize a member, or perform layout or something, you might consider using lazy initialization. This will defer the calculation of the result until the result is really needed.

Bob Dalgleish
  • 8,167
  • 4
  • 32
  • 42
1

To invoke the functions of the concrete classes in the abstract class use by lazy which allows calling non-final functions.

From: area = extractArea(room);

To: area by lazy { extractArea(room) }

GL

Braian Coronel
  • 22,105
  • 4
  • 57
  • 62