A blog about design, code and some other stuff

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

• Keep your modules and functions small
• Naming methods, properties and objects
• [whatwg] [Notifications] Constructor should not have side effects (interesting thread; Elliott Sprehn was totally right and makes very good points, I borrowed some arguments from him, a shame other participants could not “see the light”)
• Stop Writing Classes (video)
• Execution in the Kingdom of Nouns
• Designing APIs for Asynchrony