Temporal coupling vs. complex constructor

Question

I'm working through a book on writing Single Page Applications in Vanilla Javascript. (I'm working on a project where I'm not allowed to use React or any other frameworks, so I'm trying to get better at structuring vanilla SPAs.)

The book is fairly old, so I'm trying to rework some of the examples using more modern Javascript idioms. In particular, I reworked this example using a class:

export class SPA {

    constructor($container) {
        this.$container = $container;
        this.onClickSlider = this.onClickSlider.bind(this);
        this.$container.innerHTML = this.configMap.template_html;
        this.$chatSlider.setAttribute("title", this.configMap.retracted_title);
        this.$chatSlider.addEventListener("click", this.onClickSlider);
    }

    get configMap() {
        return {
            extended_height: 434,
            extended_title: "Click to retract",
            retracted_height: 16,
            retracted_title: "Click to extend",
            template_html: "<div class=\"spa-slider\"></div>"
        };
    }

    get $chatSlider() {
        if (!this._$chatSlider) {
            this._$chatSlider = this.$container.querySelector(".spa-slider");
        }
        return this._$chatSlider;
    }

    onClickSlider(event) {
        this.toggleSlider();
        event.preventDefault();
    }

    toggleSlider() {
        const slider_height = this.$chatSlider.clientHeight;

        if (slider_height === this.configMap.retracted_height) {
            this.$chatSlider.style.transition = "1s";
            this.$chatSlider.style.height = `${this.configMap.extended_height}px`;
            this.$chatSlider.setAttribute("title", this.configMap.extended_title);
        } else if (slider_height === this.configMap.extended_height) {
            this.$chatSlider.style.transition = "1s";
            this.$chatSlider.style.height = `${this.configMap.retracted_height}px`;
            this.$chatSlider.setAttribute("title", this.configMap.retracted_title);
        }
    }

}

However, this probably commits the anti-pattern on doing too much in the constructor, so I moved the initialisation of the DOM and event handlers out:

export class SPA {

    constructor($container) {
        this.$container = $container;
        this.onClickSlider = this.onClickSlider.bind(this);
    }

    connect() {
        this.$container.innerHTML = this.configMap.template_html;
        this.$chatSlider.setAttribute("title", this.configMap.retracted_title);
        this.$chatSlider.addEventListener("click", this.onClickSlider);
    }

    get configMap() {
        return {
            extended_height: 434,
            extended_title: "Click to retract",
            retracted_height: 16,
            retracted_title: "Click to extend",
            template_html: "<div class=\"spa-slider\"></div>"
        };
    }

    get $chatSlider() {
        if (!this._$chatSlider) {
            this._$chatSlider = this.$container.querySelector(".spa-slider");
        }
        return this._$chatSlider;
    }

    onClickSlider(event) {
        this.toggleSlider();
        event.preventDefault();
    }

    toggleSlider() {
        const slider_height = this.$chatSlider.clientHeight;

        if (slider_height === this.configMap.retracted_height) {
            this.$chatSlider.style.transition = "1s";
            this.$chatSlider.style.height = `${this.configMap.extended_height}px`;
            this.$chatSlider.setAttribute("title", this.configMap.extended_title);
        } else if (slider_height === this.configMap.extended_height) {
            this.$chatSlider.style.transition = "1s";
            this.$chatSlider.style.height = `${this.configMap.retracted_height}px`;
            this.$chatSlider.setAttribute("title", this.configMap.retracted_title);
        }
    }

}

However, now I have temporal coupling, since most of the methods won't work until connect() is called.

Can anyone give any advice for how to avoid the dilemma of doing things in the constructor other than assigning properties vs temporal coupling in this example?

Answer 1

No, introducing this temporal coupling is not a good idea. Constructors should create a ready to use object, or no object at all (throw an exception). So usually, a constructor will do little more than checking invariants and performing initialization.

By extracting part of the setup into the connect() method, you have made the object's API a lot more complex.

• The shape of the object now depends on which methods have been called. Ideally, the shape should stay fixed after the constructor has completed. JavaScript allows the object shape to change during runtime, but it's best not to use this flexibility unless really necessary.

• The object can now has more states that need to be considered. Good objects keep this state purely internal, the best objects have only one state and are more or less immutable.

  Previously, there were about four relevant object states ($chatSlider not initialized, element at extended height, element at retracted height, element at other height). But none of these states need to be managed by the user of the class (aside from selecting an initial element height). The $chatSlider state is also fixed within the constructor, although as written the data flow isn't obvious.

  Now, there's the additional unconnected state, and the user of this class must manually perform the state transition out of that unconnected state. Nothing will warn them that they need to do this. Such kind of API traps lead to a Pit of Despair.

When you think that a constructor is doing too much work, the solution usually isn't to defer that work until later, but to do this work before calling the constructor. Users of your class would then call some static function that does the work and finally calls the constructor.

But what work is even being done in this case?

• this.$container is assigned. That's purely initialization.
• this.onClickSlider is bound to this. Excessively clever, but purely initialization.
• $container contents are initialized. Is this forbidden in a constructor? Not really. While we aren't initializing the object's fields here, we are initializing the thing that the object represents.
• this.$chatSlider runs the querySelector as a side effect. Excessively clever, but purely initialization. I'd do this directly in the constructor rather than in the accessor.
• this.$chatSlider.foo(...) is just further initialization of the $container.

I think all of that is perfectly fine, and it doesn't make sense to extract that out of the constructor. I'd write some details a bit differently, but the original is fairly sane. My suggestion:

constructor($container) {
    $container.innerHTML = `<div class="spa-slider"></div>`;
    let $chatSlider = $container.querySelector(".spa-slider");
    $chatSlider.setAttribute("title", this.configMap.retracted_title);
    $chatSlider.addEventListener("click", this.onClickSlider.bind(this));
    // TODO: set initial $chatSlider height

    this.$container = $container;
    this.$chatSlider = $chatSlider;
}

// delete get $chatSlider() { ... }
// delete configMap.template_html

Note that the above constructor is effectively split into two parts. It looks like the first part could be extracted into factory method. But on closer inspection there are dependencies on this in that code, so it cannot be done before the object is created.

Answer 2

Initialization code is what constructors are for. Don't make me call a method every time I construct a new object. That makes me wonder what the method does that required me to call it (nothing), and feel like you're not smart enough to call it yourself.

If you like, you can break it off into a static function for code organization purposes. Remember to document it as internal use if the user shouldn't be calling it.

Alternately, if you ditch prototypes, you can put everything in the constructor:

export function ($container) {
    if (!(this instanceof arguments.callee)) return new arguments.callee($container)

    Object.defineProperty(this, "configMap", {
        get: function () {
            return {
                extended_height: 434,
                extended_title: "Click to retract",
                retracted_height: 16,
                retracted_title: "Click to extend",
                template_html: "<div class=\"spa-slider\"></div>"
            };
        }
    })

    Object.defineProperty(this, "$chatSlider", {
        get: function () {
            if (!this._$chatSlider) {
                this._$chatSlider = this.$container.querySelector(".spa-slider");
            }
            return this._$chatSlider;
        }
    })

    function onClickSlider(event) {
        this.toggleSlider();
        event.preventDefault();
    }

    this.toggleSlider = function () {
        const slider_height = this.$chatSlider.clientHeight;

        if (slider_height === this.configMap.retracted_height) {
            this.$chatSlider.style.transition = "1s";
            this.$chatSlider.style.height = `${this.configMap.extended_height}px`;
            this.$chatSlider.setAttribute("title", this.configMap.extended_title);
        } else if (slider_height === this.configMap.extended_height) {
            this.$chatSlider.style.transition = "1s";
            this.$chatSlider.style.height = `${this.configMap.retracted_height}px`;
            this.$chatSlider.setAttribute("title", this.configMap.retracted_title);
        }
    }

    this.$container = $container;
    this.onClickSlider = this.onClickSlider.bind(this);
    this.$container.innerHTML = this.configMap.template_html;
    this.$chatSlider.setAttribute("title", this.configMap.retracted_title);
    this.$chatSlider.addEventListener("click", this.onClickSlider);

}