Consider the following:
function Foo(bar) { this.bar = bar || {}; this.refresh(); }
This is a pretty common pattern - you set fields and do whatever initialization is needed. In this case, the constructor is using a refresh method, presumably so new objects and refreshed objects all get setup with the same code.
This might look fairly innocent, but what does that call to the refresh method really do? I don't know, and that's just the point. Maybe it sets some default values. Maybe it makes a database call. Maybe it makes an AJAX request for more data. This is an important thing to know when using an object written this way, as well as when you need them for testing.
What if, for example, refresh makes an AJAX request for more data and updates this.bar when it gets a response. (This could be a likely pattern when your views are data-bound, you would initially show default or cached values and then the view would update itself when new data came in.)
Now, think of testing with Foo objects. Every time you need one you have to worry about an AJAX call being fired by the constructor. Even if you have fake responses in place this can add a lot of complexity. And what about your code? If your tests are hard to maintain, will the actual code be that much better?
Another example - what if for the purposes of a test, we needed to mock the refresh method before it is called. In the JavaScript example above that would be possible if you are using prototypal class patterns (Foo.prototype.refresh = ...).
But, what if we needed to mock the refresh method for just one instance of Foo that you're particularly interested in? Now you're in trouble. True, there are some
What if your class is written like this:
function Foo(bar) { var self = new Base(); refresh(); return self; function refresh() { // Do AJAX-y things } }
Now you're really screwed. How do you control the refresh method in this case? There's no way to prevent it from being called with the actual implementation. Even making refresh public by putting it directly on self wouldn't help, you still couldn't change its behavior until after the object is constructed.
I've found that when object construction is as simple and fast as possible the world is a much happier place. Use the constructors to save off some fields (this.bar = bar) and set some default values if needed.
That's it.
Nice and simple.
If you're concerned that some necessary behavior (such as calling refresh) might be forgotten, create a factory method that will wrap that up for you:
function createAndRefreshFoo(bar) { var foo = new Foo(bar); foo.refresh(); return foo; }
It's not foolproof since your objects could still be constructed without this function, but I think it's a worthy compromise to make.
The overarching theme here is to avoid design patterns that limit you down the road. Having any sort of complex behavior in your constructors is definitely one of those patterns and I would argue it should be avoided unless absolutely necessary.
Keep things simple. Help your fellow developers. Help yourself. And leave my constructors alone!
There's a couple of problems, and I'd expect to see other flavors of both in a codebase that had lots of code like your example.
ReplyDeleteFirst: poor separation of concerns. "Fetch data" and "construct an object" are separate things, but it's common enough to see them blended together in a misguided attempt to encapsulate data access inside the class that represents the data. I'd expect that class to be coupled to its data source in other ways, such as calling a save method in various places.
Second: Poor reasoning about class invariants. If the constructor is relying on an async call to establish an invariant, it really isn't establishing it at all. Any code that works with that class has to worry about whether that async call has completed yet. I'd expect to find other issues where the class isn't guaranteeing something that it probably should.
Third: asynchronous functions should look asynchronous. (Assuming I'm not reading too much into the "Do AJAX-y things" comment.) If refresh() calls something that's actually asynchronous, then calling it from the constructor really makes no sense and the code should probably look like this:
function createAndRefreshFoo(bar, callback) {
var foo = new Foo(bar);
foo.refresh(function () {
callback(foo);
});
}
function Foo(bar) {
// ...
function refresh(callback) {
// Do AJAX-y things and call callback when done
}
}
@Steve I completely agree. This pattern has caused us a lot of headache trying to track down race conditions (which can be especially hard to notice when using wifi vs. a cell network).
ReplyDeletePersonally I think any ease of use that an auto refreshing constructor gave us was outweighed by the pain of finding bugs caused by that behavior.