Skip to content

HTTPS clone URL

Subversion checkout URL

You can clone with
or
.
Download ZIP

Loading…

Various code readability ideas #2059

Closed
wants to merge 24 commits into from

7 participants

@gigamonkey

Hey Jeremy, here are some ideas left over from my code reading refactoring plus some new ones. I'd be happy to talk about these and tweak them as needed if you're interested in incorporating them.

Note that these changes, while aimed primarily at readability, do introduce some small incompatibilities/API changes. The main one is that I got rid of the 'unset' option in preference for treating attributes passed with a undefined value as unsetting those attributes. I think this is actually righteous since json can't represent an undefined value so a model with 'foo' present but undefined will serialize the same as a model with 'foo' simply not present. I had to comment out one test case which specifically tested that you could set an attribute to undefined.

Also I axed the changed property of Model. I assume that you've kept it around for backwards compatibility so you may still be unwilling to see it go. But it seems like a wart on the public API since you're not supposed to manipulate it directly.

Anyway, had fun playing around with this. Let me know what you think.

-Peter

@philfreo philfreo commented on the diff
backbone.js
@@ -225,6 +225,113 @@
// want global "pubsub" in a convenient place.
_.extend(Backbone, Events);
+
+
+ // The ChangeTracker is used by the Model to keep track of the state of
+ // changes made to the model's attributes so at to trigger
@philfreo
philfreo added a note

typo... "so as to"

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
@jashkenas
Owner

Thanks for writing this up as a pull request -- righteous and nicely OOP-y. I'll try to grab a chance to review it soon.

@krawaller

@gigamonkey I don't have anything constructive to add, just wanted to say - really cool refactoring! Nicely done! :)

@gigamonkey

Jeremy, any chance this is actually going to get folded in?

@philfreo

This refactor is pretty cool - @jashkenas thoughts?

@jashkenas
Owner

Whoops -- sorry, I had "[backbone]" tickets filtering out of the inbox for a couple of months there. It'll be time to come back around crank through some of these soon.

@braddunbar
Collaborator

I'm afraid this may not apply after the changes to Model#set. :(

@gigamonkey

Well, it's good to merge now and it's only been sitting around for 2 months. :-(

@tbranyen
Collaborator

@gigamonkey to be fair, it's typically better to bring up an issue first and then submit a pull request so theres no wasted efforts. jQuery encourages this so that people don't feel bad if ideas are turned down after they put significant time into a pull request.

@gigamonkey

Sure. I mentioned it to Jeremy before I started. And I'm cool with whatever you guys want to do. But if some other change was already in progress when I submitted my PR it would have been cool for someone to say, "Hey check this other upcoming change out and see if you can work with it." And if the other change came after mine, it'd have been nice if either mine had already been merged or if the other author had been told about my change.

@tgriesser
Collaborator

Sorry about that @gigamonkey - I think your request came in just after the decision was made to completely rewrite set and strip out queued changes entirely, and the pull request was more directed at/responded to by Jeremy, so I figured it'd be in his court to take a look through.

@braddunbar
Collaborator

I think @tgriesser's assessment is correct, but I'm sorry you spent time on this without being informed. We're attempting to address some of these organizational failings by using milestones to track features/fixes that are currently being worked on.

@jashkenas
Owner

I have to apologize for dropping the ball on this -- the fault is entirely mine for "commissioning" it at a poor time.

That said, I think there are a couple of issues with implementing this particular direction:

  • Ideally, we wouldn't introduce a new class that exists just to namespace some of the internal Model implementation. As the ChangeTracker is only ever instantiated within the Model constructor, it feels like it should simply be methods directly on the model itself.

  • That unset change is righteous -- you have a great point about the JSON, and we should definitely do that.

  • The model.changed property should stick around. It's useful, and is an intentionally-exposed part of the internals much like model.attributes is, or even model.id. Read-only reference properties.

So, closing, but thanks much!

@jashkenas jashkenas closed this
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Commits on Jan 1, 2013
  1. @gigamonkey

    Rename set -> _set

    gigamonkey authored
  2. @gigamonkey
  3. @gigamonkey
  4. @gigamonkey
  5. @gigamonkey
  6. @gigamonkey
  7. @gigamonkey

    Rename _set -> _change.

    gigamonkey authored
  8. @gigamonkey
  9. @gigamonkey

    More furniture moving.

    gigamonkey authored
  10. @gigamonkey
  11. @gigamonkey

    Finishing comment.

    gigamonkey authored
  12. @gigamonkey
  13. @gigamonkey
  14. @gigamonkey
  15. @gigamonkey
  16. @gigamonkey
  17. @gigamonkey

    Rename method.

    gigamonkey authored
  18. @gigamonkey
  19. @gigamonkey
  20. @gigamonkey

    Using call counts

    gigamonkey authored
  21. @gigamonkey

    Some tidying.

    gigamonkey authored
  22. @gigamonkey
  23. @gigamonkey
  24. @gigamonkey
This page is out of date. Refresh to see the latest.
Showing with 171 additions and 77 deletions.
  1. +158 −75 backbone.js
  2. +13 −2 test/model.js
View
233 backbone.js
@@ -225,6 +225,113 @@
// want global "pubsub" in a convenient place.
_.extend(Backbone, Events);
+
+
+ // The ChangeTracker is used by the Model to keep track of the state of
+ // changes made to the model's attributes so at to trigger
@philfreo
philfreo added a note

typo... "so as to"

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
+ // 'change:attribute' and 'change' events at the right times. This is
+ // moderately involved since a single call to a model mutatator (set,
+ // unset, or clear) can trigger events whose handlers may further mutate
+ // the model but we want to treat each top-level call to a mutator as a
+ // single change.
+
+ var ChangeTracker = function(model) {
+ this.model = model;
+ this.before = void 0;
+ this.changed = {};
+ this.attributeChangesTriggered = false;
+ this.changesets = [];
+ };
+
+ ChangeTracker.prototype = {
+
+ enter: function () {
+ this.changesets.push([]);
+ if (this.changesets.length === 1) {
+ this.before = _.clone(this.model.attributes);
+ this.changed = {};
+ this.attributeChangesTriggered = false;
+ }
+ },
+
+ exit: function (options) {
+ var i, attr, model, changes, current;
+
+ if (!options.silent) {
+ model = this.model;
+ changes = this.changesets[this.changesets.length - 1];
+
+ if (changes.length) {
+ current = model.attributes;
+ for (i in changes) {
+ attr = changes[i];
+ model.trigger('change:' + attr, model, current[attr], options);
+ }
+ this.attributeChangesTriggered = true;
+ }
+
+ // Only do this when exiting from the top level call to a mutator.
+ if (this.changesets.length === 1) {
+ // Keep triggering the change event until things quiesce. Thus one
+ // call to a mutator may result in multiple change events if the
+ // handler for on change event further mutates the model.
+ while (this.attributeChangesTriggered) {
+ this.attributeChangesTriggered = false;
+ model.trigger('change', model, options);
+ }
+ }
+ }
+ this.changesets.pop();
+ },
+
+ recordChange: function (attr, currentVal, val) {
+ var changes = this.changesets[this.changesets.length - 1];
+ if (!_.isEqual(currentVal, val)) changes.push(attr);
+ _.isEqual(this.before[attr], val) ? delete this.changed[attr] : this.changed[attr] = val;
+ },
+
+ forgetChanges: function () { this.changed = {}; },
+
+ hasChanged: function(attr) {
+ if (attr == null) return !_.isEmpty(this.changed);
+ return _.has(this.changed, attr);
+ },
+
+ inChange: function () { return this.changesets.length > 0; },
+
+ changedAttributes: function(diff) {
+ if (!diff) return this.hasChanged() ? _.clone(this.changed) : false;
+
+
+ // In this case we are not actually asking if anything has changed but
+ // rather, whether if we were to set(diff) it would result in a
+ // change. In the midst of a change we want to compare the prospective
+ // values to the values prior to the start of the change (i.e.
+ // this.before) but once all changes are finished we want to compare
+ // to the current values (i.e. this.model.attributes). I'm not sure
+ // why that actually is; why, in a callback, wouldn't you want to
+ // compare to whatever the current values are? -Peter Seibel
+ var attr, val, changed = false;
+ var old = this.inChange() ? this.before : this.model.attributes;
+
+ for (attr in diff) {
+ val = diff[attr];
+ if (!_.isEqual(old[attr], val)) {
+ (changed || (changed = {}))[attr] = val;
+ }
+ }
+ return changed;
+ },
+
+ previous: function(attr) {
+ if (attr == null || !this.before) return null;
+ return this.before[attr];
+ },
+
+ previousAttributes: function() { return _.clone(this.before); }
+
+ };
+
// Backbone.Model
// --------------
@@ -240,17 +347,15 @@
if (defaults = _.result(this, 'defaults')) {
attrs = _.defaults({}, attrs, defaults);
}
+ this._changeTracker = new ChangeTracker(this);
this.set(attrs, options);
- this.changed = {};
+ this._changeTracker.forgetChanges();
this.initialize.apply(this, arguments);
};
// Attach all inheritable methods to the Model prototype.
_.extend(Model.prototype, Events, {
- // A hash of attributes whose current and previous value differ.
- changed: null,
-
// The default name for the JSON `id` attribute is `"id"`. MongoDB and
// CouchDB users may want to set this to `"_id"`.
idAttribute: 'id',
@@ -287,77 +392,62 @@
// ----------------------------------------------------------------------
- // Set a hash of model attributes on the object, firing `"change"` unless
- // you choose to silence it.
- set: function(key, val, options) {
- var attr, attrs, unset, changes, silent, changing, prev, current;
- if (key == null) return this;
-
- // Handle both `"key", value` and `{key: value}` -style arguments.
- if (typeof key === 'object') {
- attrs = key;
- options = val;
- } else {
- (attrs = {})[key] = val;
- }
+ // The primitive method for changing the attribute values in the
+ // model. Used by set, unset, and clear. Unless the silent option
+ // is passed, fires change:<foo> events for each changed attribute
+ // and a final change event.
+ _change: function(attrs, options) {
+ var attr, val, silent, tracker, current;
options || (options = {});
+ silent = options.silent;
// Run validation.
if (!this._validate(attrs, options)) return false;
- // Extract attributes and options.
- unset = options.unset;
- silent = options.silent;
- changes = [];
- changing = this._changing;
- this._changing = true;
-
- if (!changing) {
- this._previousAttributes = _.clone(this.attributes);
- this.changed = {};
- }
- current = this.attributes, prev = this._previousAttributes;
-
// Check for changes of `id`.
if (this.idAttribute in attrs) this.id = attrs[this.idAttribute];
- // For each `set` attribute, update or delete the current value.
- for (attr in attrs) {
- val = attrs[attr];
- if (!_.isEqual(current[attr], val)) changes.push(attr);
- if (!_.isEqual(prev[attr], val)) {
- this.changed[attr] = val;
- } else {
- delete this.changed[attr];
- }
- unset ? delete current[attr] : current[attr] = val;
- }
+ tracker = this._changeTracker;
+ current = this.attributes;
- // Trigger all relevant attribute changes.
- if (!silent) {
- if (changes.length) this._pending = true;
- for (var i = 0, l = changes.length; i < l; i++) {
- this.trigger('change:' + changes[i], this, current[changes[i]], options);
+ tracker.enter();
+ try {
+ for (attr in attrs) {
+ val = attrs[attr];
+ tracker.recordChange(attr, current[attr], val);
+ _.isUndefined(val) ? delete current[attr] : current[attr] = val;
}
+ } finally {
+ tracker.exit(options);
}
- if (changing) return this;
- if (!silent) {
- while (this._pending) {
- this._pending = false;
- this.trigger('change', this, options);
- }
- }
- this._pending = false;
- this._changing = false;
return this;
},
+ // Set a hash of model attributes on the object, firing `"change"` unless
+ // you choose to silence it.
+ set: function(first, second, third) {
+ // Handle both `"key", value` and `{key: value}` -style arguments.
+ var attrs, options;
+
+ if (typeof first === 'object') {
+ attrs = first;
+ options = second;
+ } else {
+ (attrs = {})[first] = second;
+ options = third;
+ }
+
+ return this._change(attrs, options);
+ },
+
// Remove an attribute from the model, firing `"change"` unless you choose
// to silence it. `unset` is a noop if the attribute doesn't exist.
unset: function(attr, options) {
- return this.set(attr, void 0, _.extend({}, options, {unset: true}));
+ var attrs = {};
+ attrs[attr] = void 0;
+ return this._change(attrs, options);
},
// Clear all attributes on the model, firing `"change"` unless you choose
@@ -365,14 +455,17 @@
clear: function(options) {
var attrs = {};
for (var key in this.attributes) attrs[key] = void 0;
- return this.set(attrs, _.extend({}, options, {unset: true}));
+ return this._change(attrs, options);
},
+ // XXX: that comments on the next few methods are not quite right
+ // because changes are recorded even when the mutating methods are
+ // called with a silent option and thus no change event is fired.
+
// Determine if the model has changed since the last `"change"` event.
// If you specify an attribute name, determine if that attribute has changed.
hasChanged: function(attr) {
- if (attr == null) return !_.isEmpty(this.changed);
- return _.has(this.changed, attr);
+ return this._changeTracker.hasChanged(attr);
},
// Return an object containing all the attributes that have changed, or
@@ -380,30 +473,20 @@
// parts of a view need to be updated and/or what attributes need to be
// persisted to the server. Unset attributes will be set to undefined.
// You can also pass an attributes object to diff against the model,
- // determining if there *would be* a change.
+ // determining if there *would be* a change. If this method is called
+ // from a change event callback the changes are considered relative to
+ // the state of the model before the mutation started.
changedAttributes: function(diff) {
- if (!diff) return this.hasChanged() ? _.clone(this.changed) : false;
- var val, changed = false;
- var old = this._changing ? this._previousAttributes : this.attributes;
- for (var attr in diff) {
- if (_.isEqual(old[attr], (val = diff[attr]))) continue;
- (changed || (changed = {}))[attr] = val;
- }
- return changed;
+ return this._changeTracker.changedAttributes(diff);
},
// Get the previous value of an attribute, recorded at the time the last
// `"change"` event was fired.
- previous: function(attr) {
- if (attr == null || !this._previousAttributes) return null;
- return this._previousAttributes[attr];
- },
+ previous: function(attr) { return this._changeTracker.previous(attr); },
// Get all of the attributes of the model at the time of the previous
// `"change"` event.
- previousAttributes: function() {
- return _.clone(this._previousAttributes);
- },
+ previousAttributes: function() { return this._changeTracker.previousAttributes(); },
// ---------------------------------------------------------------------
View
15 test/model.js
@@ -126,7 +126,7 @@ $(document).ready(function() {
var foo = new Backbone.Model({p: 1});
var bar = new Backbone.Model({p: 2});
- bar.set(foo.clone().attributes, {unset: true});
+ bar.clear();
equal(foo.get('p'), 1);
equal(bar.get('p'), undefined);
});
@@ -244,7 +244,7 @@ $(document).ready(function() {
test("set falsy values in the correct order", 2, function() {
var model = new Backbone.Model({result: 'result'});
model.on('change', function() {
- equal(model.changed.result, void 0);
+ equal(model.changedAttributes()['result'], void 0);
equal(model.previous('result'), false);
});
model.set({result: void 0}, {silent: true});
@@ -644,10 +644,21 @@ $(document).ready(function() {
model.unset('x');
});
+ /*
+ I have to take out this test since I'm now treating assigning an
+ undefined value to an attribute as the same as unsetting it. I.e.
+ it's no longer possible for an attribute to exist but have an
+ undefined value. Which seems righteous to me since there's no way
+ to send an undefined value in JSON and thus a model with present
+ but undefined values is the same, from a JSON point of view, as a
+ model with those values simply not present. -Peter
+
+
test("set: undefined values", 1, function() {
var model = new Backbone.Model({x: undefined});
ok('x' in model.attributes);
});
+ */
test("hasChanged works outside of change events, and true within", 6, function() {
var model = new Backbone.Model({x: 1});
Something went wrong with that request. Please try again.