2014.04.13

Constructors should not cause side effects

Going to talk about something simple that took me a while to figure out I was doing it wrong. Don’t know when I realized it was a big mistake, I just know that it is something I’ve been trying to avoid for a while.

To begin, I’m not the first one to get into this conclusion. Many people before thought about the same thing, it is even one of the JSLint rules (“Do not use ‘new’ for side effects”). The thing is, I don’t think most people know the reason why this should be avoided; so let me explain why.

Constructors are not verbs

I believe that methods should be named as verbs, to make it clear that they perform actions.

new XMLHttpRequest doesn’t fire the request, new HTMLDivElement() shouldn’t append it to the document, … – Elliott Sprehn

There is a very important principle called Command-Query Separation, this principle states that functions should either be a command that performs an action, or a query that returns data to the caller.. There is also another very important principle for SOLID design, that is the Single Responsibility Principle. According to the SRP, each method, module and class should have a single responsibility; increasing the cohesion.

So, why am I talking about CQS, SRP and cohesion? Because constructors have a very specific responsibility; constructors are nothing more than an abstraction used to create a new instance. It should do the minimum work necessary to setup the object; basically just setting default properties and to allow some sort of configuration, just so each instance have it’s own values. In many cases constructors should be empty (no logic at all).

Harder to test

If your constructor performs any kind of action, it becomes really hard to unit test it. Imagine this scenario:

// WARNING: this is an anti-pattern!
function Dialog(opts) {
  this.msg = opts.msg;
  this.size = opts.size || 'small';
  // ... imagine a few more options
  this.element = document.createElement('div');
  // ... imagine a few more lines of code building the element structure
  this._setupEventListeners();
  // and automatically appending the element to the DOM
  document.body.appendChild(this.element);
}

Just think about it for a moment.. How would you unit test it?

If you let the constructor with a single responsibility, instead:

function Dialog(opts) {
  // we also remove the "message" from the constructor, a dialog should be able
  // to display multiple messages.
  this.size = opts.size || 'small';
}

Testing it becomes easy, since you are sure that it is only used as a way to set the options.

describe('Dialog constructor', function() {
  it('should set default options if none provided', function() {
    var dialog = new Dialog();
    expect(dialog.size).toEqual('small');
  });

  it('should override default options if provided', function() {
    var dialog = new Dialog({ size: 'large' });
    // PS: notice that `size` could be set later since it's a public property,
    // so in many cases it's OK to have empty constructors.
    expect(dialog.size).toEqual('large');
  });
});

This also allows us to create instances during the test setup without worrying if the environment is at the proper state.

Specially if action is asynchronous!

It is not uncommon to have widgets that depends on data coming from a server. This is definitely the biggest offender.

// WARNING: this is an anti-pattern!
function Weather(cityName) {
  request('http://example.com/?city='+ cityName, this._setup.bind(this));
}

Now you surely can’t test the Weather without mocking the request function, and the API consumer won’t have any idea of when the Weather is really ready to be used. You also have no option to delay the AJAX request. It is a disaster waiting to happen (ZALGO!).

You can’t pass a reference to the instance around and expect it to just work. All the hacks to circumvent this problem (events, command queue, safeguards…) will lead to code that is hard to maintain, hard to reuse and hard to test. So please don’t do it!

The proper solution would be to add a second method that actually triggers the AJAX request when needed. So the consumer of this API would do:

var saoPauloWeather = new Weather('sao-paulo');
// the `get` method can have a callback which makes intent way clearer and
// decouples the instantiation from the AJAX request, allowing us to defer the
// execution till needed. You could even cache the result so subsequent calls
// to `get` would execute on the next tick (callback should always be called
// asynchronously to avoid mistakes)
saoPauloWeather.get(onWeatherLoad);

This have a very good side effect of allowing us to mock the get method during tests to bypass the AJAX request altogether if needed.

How to easily detect offenders

If you are simply creating an instance but not calling any method on the instance itself afterwards, it means it should probably not be a constructor to begin with.

// WARNING: anti-pattern!
function showMessage() {
  new Dialog('this is an anti-pattern!');
}

To really justify the constructor your code should look more like this:

var dialog = new Dialog();
dialog.showMessage('this is the way to do it');
// ... later in the code
dialog.close();

If your class is only used to perform a single action and it’s immediately disposed, it should be just a regular function instead.

“Sometimes, the elegant implementation is a function. Not a method. Not a class. Not a framework. Just a function.” – John Carmack

Always good to remember that you can use closures to hold state/data; you don’t need instances in many cases.

function showDialog(msg) {
  var element = document.createElement('div');
  // ... imagine more code here to setup the dialog
  closeButton.addEventListener('click', function() {
    // because of the closure you still have access to the "element" variable,
    // this technique can be useful in many cases.
    element.parentNode.removeChild(element);
  });
}

Conclusion

I hope it’s clear what are some of the drawbacks of adding too much logic inside constructors, and also some of the benefits of splitting the logic into discrete methods. It might feel more bureaucratic at the beginning but in the end it will pay-off, specially if you keep it consistent.

My golden rule is: never execute code automatically, let the API consumer to call things explicitly; it increases flexibility and allows more control.

That’s it for now!

Further reading/watching


Comments

Rafael Rinaldi

I only realized that is a mess to keep too much logic inside constructors when I started working with TDD. And if it's hard to test, you're probably doing it wrong.

Just one thing that's not clear for me: you don't think constructors should be used, or you just think that they shouldn't hold any logic inside them?

The dialog example is great, I usually create objects in this case. I think it's very clear when reading the code that dialog is a new object that has specific behaviors. I don't think that it's a good idea to just use function calls in this case as you suggest in the second example. Isn't it hard to test anyway?

I avoided constructors in JS for a while, but nowadays I use it whenever I feel the need for multiple instances.. I just think they should not hold a lot of logic, I think we should defer the "heavy" logic till needed (maybe add another method "init()" or "setup()"), since that makes it easier to test and increases flexibility.

I've been using Functional Programming techniques very often, specially to manipulate data. I rarely couple data with behavior (breaking the OOP paradigm). But for UI widgets, I still feel that constructors/prototype are a good way to organize the code, since it is very common to need multiple instances of same widget and it is also very common for widgets to have "state" (is it open?); not that you couldn't solve the problem in a different way (specially since you have the DOM to store some data/state), but I still like to keep all logic/data in the JS if possible, maybe one day I'll change my opinion.

You could still test the dialog code even without instances, you just need to break the code into more functions and test each one individually. It was just to show that closures can replace objects in many cases.

I just found this article and found it would be great to read, if ppl are looking to deep dive on JS constructors:

http://raganwald.com/2014/07/09/javascript-constructor-problem.html

[…] When I Was Younger 0:25:57 People Crying Everywhere 0:27:21 Shipping The New GitHub Issues 0:34:10 Constructors Should Not Cause Side Effects 0:54:20 Swiffit 0:57:35 esformatter 0:58:57 Front In BH 0:59:08 Anúncio bombástico. 1:02:17 […]

Leave a Comment

Please post only comments that will add value to the content of this page. Please read the about page to understand the objective of this blog and the way I think about it. Thanks.

Comments are parsed as Markdown and you can use basic HTML tags (a, blockquote, cite, code, del, em, strong) but some code may be striped if not escaped (specially PHP and HTML tags that aren't on the list). Line and paragraph breaks automatic. Code blocks should be indented with 4 spaces.