Is it bad practice to assign computed properties directly on a Model?

[elisechant]

Is it an anti-pattern to define computed properties on a Model or should computed properties only be defined in a Controller? For example:

App.Person = DS.Model.extend({
	first: DS.attr(),
	last: DS.attr(),
	fullName: function() {
		return [this.get('first'), this.get('last')].join(' '); 
	}.property('first', 'last')
});

as opposed to:

App.Person = DS.Model.extend({
	first: DS.attr(),
	last: DS.attr(),	
});

App.PersonController = Ember.Controller.extend({
	fullName: function() {
		return [this.get('first'), this.get('last')].join(' '); 
	}.property('first', 'last')
});

[funtusov]

In my view, if it’s an intrinsic attribute of the model (like in the example, a full name is a characteristic of a person regardless of context), it should be defined in the model, to avoid duplication.

[elisechant]

@funtusov yes, I would agree with that.

[denzo]

@funtusov I would have to disagree with you.

Here’s a quote from Ember guide:

In Ember.js, controllers allow you to decorate your models with display logic. In general, your models will have properties that are saved to the server, while controllers will have properties that your app does not need to save to the server. http://emberjs.com/guides/controllers/

‘fullName’ property is simply a decoration of the model and belongs in the controller. Maintaining a clear separation of concerns is what makes the structure of Ember so powerful.

Great question Elise

[funtusov]

@denzo Interesting, thanks for pointing to the guide.

However, could you explain the reason why having fullName defined once in a model is worse than multiple times in several controllers?

[kylecoberly]

As an example, think about the different ways you could implement fullName.

First Last FIRST LAST Last, First

Etc. Would each of those go in the model? You might only use one now, but what about two years from now? Data decorations are primarily concerned with the UI, so you generate and store them closer to the UI. It helps with change management.

In practice, they both function, and the wrong way is even quicker to develop. Code spends more time in maintenance than development though, so the best practice is to develop for easy maintenance.

[elisechant]

@kylecoberly I tend to agree that “data decorations primarily concerned with UI … (should be) stored close to the UI”, but I lean towards @funtusov’s reasoning - I agree that in Ember the model is the best single, central place of reference, as a model may share many controllers.

I see Ember as a framework offering built strongly upon the concept of data models and therefore that the centricity of its data is of high importance.

And further, since these such values are not being pushed back to the server I can’t reasonably justify the performance or data integrity arguments.

I’ve noticed that this is also how the discuss.emberjs.com source handles it: https://github.com/discourse/discourse/blob/master/app/assets/javascripts/discourse/models/post.js @denzo @kylecoberly would be interested to hear thoughts specifically with regards to this architecture?

[kylecoberly]

IMO, the pattern is that model is a single source of TRUTH. A fullName is not truth, it’s an opinion on how two pieces of truth should be combined. It’s entirely possible that this distinction is trivial to your application (or Discourse), or that “fullName” is a well-enough understood computation that you have nothing to gain but performance by storing it in your model. Bottom-line, if you do this, absolutely no one on your team is going to call you a dummy, and you’re probably not going to lose a job if you answer this on an interview question.

Your original question was about patterns, and I would rules-lawyer about this absolutely being an anti-pattern. I want whatever is controlling my data to only care about what the lowest level of truth is. What fullName, or mailingAddress, or combinedSocial might be in my application could be completely different in someone else’s, or in mine a month from now, or in this page when I change how it works. This pattern allows you to have one version of fullName on one route, and a different one on another route. This is super-desirable when those routes are developed by different people who don’t talk to each other as much as they should.

Really, the entire philosophy of data design is based around some truly conspiratorial worries- “What if three middle names becomes the standard?” or “What if global addresses become single integers?” Good application design is impervious to these changes because of patterns.

In the brutally honest real-world, no one is going to use your application except you and maybe your client. Storing a computation in the model (why not the database!) is faster. So is denormalizing. Patterns will protect you from when the entire use-case shifts. It’s a 100% judgement call whether the pros outweigh the cons on that for you.

The pattern is to keep any opinionated, domain-specific data implementation away from persistence as much as possible. For me, that would include ember models.

[denzo]

All the data manipulation ideally should be managed by a Controller. That is its purpose. If you have some on your models and some on your controllers that could create an undesirable mess. I have personally experienced that and it created confusion. Remember, that models can’t access controller’s properties.

Mixins or object inheritance will help you to avoid repeating it in several controllers.

In regards to discourse’s models, it would be interesting to hear from @eviltrout

[eviltrout]

This is one of those cases where I have to remind people that the Discourse code base is far from perfect! It’s gone through years of Ember updates and patterns and while we try to fix things as we go along, there are always things I’m unhappy with.

I believe computed properties are almost always the controller’s job. Going forward I am trying to make all models super lean and extend display logic to the controller layer.

Having said that, you can make good arguments for putting them in the model that I wouldn’t disagree with. Let’s say it’s a property that is expensive to calculate but you are going to show over and over again? I can’t honestly think of one off the top of my head, but that would be a good thing to put in a model as a CP for the caching.

[elisechant]

@denzo I found a nice design pattern executing your suggestion to abstract Computed Properties to reusable Mixins at this repo https://github.com/developing-an-emberjs-edge/ember-trackr

https://github.com/developing-an-emberjs-edge/ember-trackr/blob/master/js/mixins/needs_users.js

https://github.com/developing-an-emberjs-edge/ember-trackr/blob/master/js/controllers/ticket_controller.js

I’m now satisfied that this is the most ideal way to assign reusable computed properties without assigning directly on either a Model or a Controller. Although it’s fair that we all agree that properties may still be assigned directly to Controllers or Models if they are not be reusable or significant enough to afford caching respectively.

[elisechant]

An alternative using ObjectProxy as a decorator:

App.User = DS.Model.extend({
    staffNumber: DS.attr('string'),
    firstName: DS.attr('string'),
    lastName: DS.attr('string')
});

var UserDecorator = Ember.ObjectProxy.extend({
    fullName: function() {
        return [this.get('firstName'), this.get('lastName')].join(' ');
    }.property('firstName', 'lastName')
});

App.UserRoute = Ember.Route.extend({
   model: function(params) {
       return this.get('store').find('user', params.user_id).then(function(user) {
           return UserDecorator.create({
               content: user
           })
       });
   }
});

[cyril]

You probably don’t want multiple controllers handling the same model. You can have a dependency between controllers with needs or you can specify a controller in the route with controllerName.

[ulisesrmzroche]

You could have a currentUser controller, holding a user model, or your user route’s backing controller, which holds the user model How you structure this is more of a UI concern, and doesn’t always have to map 1:1.

[ulisesrmzroche]

Computed Properties could go in both models or controllers, they are very useful, for more than just decorating. There is no reason to constrain them to a specific layer. If they feel like they belong on a model, just put it there. Another big win is that now the user object has the full name property everywhere it is used instead of on a case-by-case basis per controller.