{"action":"created","issue":{"url":"https://api.github.com/repos/jdanyow/aurelia-breeze/issues/11","labels_url":"https://api.github.com/repos/jdanyow/aurelia-breeze/issues/11/labels{/name}","comments_url":"https://api.github.com/repos/jdanyow/aurelia-breeze/issues/11/comments","events_url":"https://api.github.com/repos/jdanyow/aurelia-breeze/issues/11/events","html_url":"https://github.com/jdanyow/aurelia-breeze/issues/11","id":66087037,"number":11,"title":"Tweaks to BreezeObservationAdapter methods","user":{"login":"wardbell","id":129061,"avatar_url":"https://avatars.githubusercontent.com/u/129061?v=3","gravatar_id":"","url":"https://api.github.com/users/wardbell","html_url":"https://github.com/wardbell","followers_url":"https://api.github.com/users/wardbell/followers","following_url":"https://api.github.com/users/wardbell/following{/other_user}","gists_url":"https://api.github.com/users/wardbell/gists{/gist_id}","starred_url":"https://api.github.com/users/wardbell/starred{/owner}{/repo}","subscriptions_url":"https://api.github.com/users/wardbell/subscriptions","organizations_url":"https://api.github.com/users/wardbell/orgs","repos_url":"https://api.github.com/users/wardbell/repos","events_url":"https://api.github.com/users/wardbell/events{/privacy}","received_events_url":"https://api.github.com/users/wardbell/received_events","type":"User","site_admin":false},"labels":[],"state":"open","locked":false,"assignee":null,"milestone":null,"comments":17,"created_at":"2015-04-03T06:19:00Z","updated_at":"2015-04-08T19:32:36Z","closed_at":null,"body":"While investigating splice-vs-null setting (see [this issue](https://github.com/jdanyow/aurelia-breeze/issues/10)), I tripped over a few adjustments you might want to make.\r\n\r\nAll of them can be found in [this gist](https://gist.github.com/wardbell/d843cd7863f70a2a5b8b).\r\n\r\n## subscribe\r\n\r\nInstead of repeatedly executing `this.callbacks[propertyName]`, I would capture that array. Your splicing version would look like this:\r\n\r\n\tfunction subscribe(propertyName, callback){\r\n\t\tvar callbacks = this.callbacks[propertyName];\r\n\t\tif (!callbacks) {\r\n\t\t  this.callbacks[propertyName] = [callback];\r\n\t\t} else if (callbacks.indexOf(callback) === -1) {\r\n\t\t  callbacks.push(callback);\r\n\t\t} else {\r\n\t\t  return; // already registered; ignore.\r\n\t\t}\r\n\r\n\t\tif (this.callbackCount === 0) {\r\n\t\t  this.subscription = this.obj.entityAspect.propertyChanged.subscribe(this.handleChanges.bind(this));\r\n\t\t}\r\n\r\n\t\tthis.callbackCount += 1;\r\n\r\n\t\treturn this.unsubscribe.bind(this, propertyName, callback);\r\n\t}\r\n\r\n>I think you are correct to ignore an attempt to re-register the same callback function rather than throw. OTOH, I prefer moving the `elses` around because I find it easier to read a positive (`=== -1`) rather than a negative (`!=== -1`). Don't you?\r\n\r\n## unsubscribe\r\nInstead of calculating the index of the callback twice, I would call it once.\r\n\r\nI guard against a failure to find the callback for a particular property, whether that's because the callback isn't registered or because there aren't any subscriptions for that property. Both cases I treat as \"no-ops\".\r\n\r\nI also think it is safer (from a memory leak perspective) to zap the `this.callbacks` when there are no more subscriptions.\r\n\r\nMy revised version looks like this:\r\n\r\n\tfunction unsubscribe(propertyName, callback) {\r\n\t\tvar callbacks = this.callbacks[propertyName];\r\n\t\tif (!callbacks) { return; } // no subscriptions for this property; ignore.\r\n\t\tvar index = callbacks.indexOf(callback);\r\n\t\tif (index === -1) { return; } // this callback was not found; ignore.\r\n\t\tcallbacks.splice(index, 1);\r\n\t\tthis.callbackCount -= 1;\r\n\t\tif (this.callbackCount === 0) {\r\n\t\t  this.obj.entityAspect.propertyChanged.unsubscribe(this.subscription);\r\n\t\t  this.callbacks={};\r\n\t\t}\r\n\t}\r\n\r\n>Per Crockford, I'm using `this.callbackCount += 1` and ` this.callbackCount -= 1` instead of `this.callbackCount++` and `this.callbackCount--`.  That change is *really* picky.\r\n"},"comment":{"url":"https://api.github.com/repos/jdanyow/aurelia-breeze/issues/comments/91012549","html_url":"https://github.com/jdanyow/aurelia-breeze/issues/11#issuecomment-91012549","issue_url":"https://api.github.com/repos/jdanyow/aurelia-breeze/issues/11","id":91012549,"user":{"login":"wardbell","id":129061,"avatar_url":"https://avatars.githubusercontent.com/u/129061?v=3","gravatar_id":"","url":"https://api.github.com/users/wardbell","html_url":"https://github.com/wardbell","followers_url":"https://api.github.com/users/wardbell/followers","following_url":"https://api.github.com/users/wardbell/following{/other_user}","gists_url":"https://api.github.com/users/wardbell/gists{/gist_id}","starred_url":"https://api.github.com/users/wardbell/starred{/owner}{/repo}","subscriptions_url":"https://api.github.com/users/wardbell/subscriptions","organizations_url":"https://api.github.com/users/wardbell/orgs","repos_url":"https://api.github.com/users/wardbell/repos","events_url":"https://api.github.com/users/wardbell/events{/privacy}","received_events_url":"https://api.github.com/users/wardbell/received_events","type":"User","site_admin":false},"created_at":"2015-04-08T19:32:36Z","updated_at":"2015-04-08T19:32:36Z","body":"Confirmed in .NET\r\n\r\n    private int count = 0;\r\n    private event System.EventHandler _show;\r\n\r\n    // Add event handlers to Show event.\r\n    _show += HiThere;\r\n    _show += HiThere;\r\n    _show += HiThere;\r\n\r\n     // Invoke the event.\r\n     _show(this, new System.EventArgs());\r\n\r\n    void HiThere(object sender, System.EventArgs args) {\r\n         if (count > 0) {\r\n           throw new System.Exception(\"called twice\"); // and it does\r\n        }\r\n        System.Console.WriteLine(\"Hi there \" + count++);\r\n    }"}}