Create EndpointsController.

Create EndpointsController.

Encapsulate the endpoints event handling.

https://code.launchpad.net/~bac/juju-gui/endpointsmgr/+merge/156961

(do not edit description out of merge proposal)

[bac, 2013-04-03 20:38:40]

Please take a look.

[bac, 2013-04-03 20:51:52]

Please take a look.

[jeff.pihach, 2013-04-03 22:18:26]

This is looking great thanks for putting in so much effort! There are a few
improvements that I've mentioned below. let me know if you have any questions.

https://codereview.appspot.com/8338043/diff/5001/app/store/endpoints.js
File app/store/endpoints.js (right):

https://codereview.appspot.com/8338043/diff/5001/app/store/endpoints.js#newco...
app/store/endpoints.js:24: 
You should create an `initializer` method and then place in it the properties
mentioned in following comments.

https://codereview.appspot.com/8338043/diff/5001/app/store/endpoints.js#newco...
app/store/endpoints.js:37: _subscriptions: [],
You don't want this here - then it will be shared across all instances of this
class. Define in the intializer:

this._subscriptions = [];

https://codereview.appspot.com/8338043/diff/5001/app/store/endpoints.js#newco...
app/store/endpoints.js:39: endpointsMap: {},
I'd probably move this to the initializer as well as you probably don't want to
share the map across instances.

https://codereview.appspot.com/8338043/diff/5001/app/store/endpoints.js#newco...
app/store/endpoints.js:56: db.services.after('reset', this.reset, this);
You're looping through the subscriptions in the unbind but you aren't adding
these handlers to the subscription array.

this._subscriptions.push(...attachevent...); x5

https://codereview.appspot.com/8338043/diff/5001/app/store/endpoints.js#newco...
app/store/endpoints.js:78: destroy: function() {
rename this to `destructor`. On Base derived classes the destructor is called
when you call destroy() on the instance.

https://codereview.appspot.com/8338043/diff/5001/app/store/endpoints.js#newco...
app/store/endpoints.js:121: if (!Y.Lang.isValue(charm)) {
if (charm) { should suffice here.

https://codereview.appspot.com/8338043/diff/5001/app/store/endpoints.js#newco...
app/store/endpoints.js:175: delete(this.endpointsMap[svcName]);
I think the official syntax for delete is `delete this.endpointsMap[svcName];`

https://codereview.appspot.com/8338043/diff/5001/test/test_endpoints.js
File test/test_endpoints.js (right):

https://codereview.appspot.com/8338043/diff/5001/test/test_endpoints.js#newco...
test/test_endpoints.js:34: app = new Y.juju.App({env: env});
these app instances should be destroyed in the afterEach.

https://codereview.appspot.com/8338043/diff/5001/test/test_endpoints.js#newco...
test/test_endpoints.js:143: controller = new EndpointsController();
this is not being destroyed in an afterEach

https://codereview.appspot.com/8338043/diff/5001/test/test_endpoints.js#newco...
test/test_endpoints.js:303: controller = app.endpointsController;
did you mean to create new instances here? If so they will then need to be
destroyed in the afterEach :-)

https://codereview.appspot.com/8338043/diff/5001/test/test_environment_view.js
File test/test_environment_view.js (right):

https://codereview.appspot.com/8338043/diff/5001/test/test_environment_view.j...
test/test_environment_view.js:798: var endpointsController = new Y.Base();
Should you not be creating a new instance of your endpointsController instead?

https://codereview.appspot.com/8338043/diff/5001/test/test_environment_view.j...
test/test_environment_view.js:842: assert.equal('hidy ho', endpointsController);
view.destroy();

[bcsaller, 2013-04-03 22:48:03]

LGTM w/Jeff's changes. 

He is right about tracking the subscription events. Y.on and friends return the
subscription object which can be used to unbind. 

Thanks for the follow through on this. I like the way its turned out better.
Much nicer encapsulation and better event handling.

[bac, 2013-04-04 13:37:49]

Please take a look.

https://codereview.appspot.com/8338043/diff/5001/app/store/endpoints.js
File app/store/endpoints.js (right):

https://codereview.appspot.com/8338043/diff/5001/app/store/endpoints.js#newco...
app/store/endpoints.js:24: 
On 2013/04/03 22:18:27, jeff.pihach wrote:
> You should create an `initializer` method and then place in it the properties
> mentioned in following comments.

I've created the initializer but don't understand the second half of your
comment.

https://codereview.appspot.com/8338043/diff/5001/app/store/endpoints.js#newco...
app/store/endpoints.js:37: _subscriptions: [],
On 2013/04/03 22:18:27, jeff.pihach wrote:
> You don't want this here - then it will be shared across all instances of this
> class. Define in the intializer:
> 
> this._subscriptions = [];

Done.

https://codereview.appspot.com/8338043/diff/5001/app/store/endpoints.js#newco...
app/store/endpoints.js:39: endpointsMap: {},
On 2013/04/03 22:18:27, jeff.pihach wrote:
> I'd probably move this to the initializer as well as you probably don't want
to
> share the map across instances.

Done.

https://codereview.appspot.com/8338043/diff/5001/app/store/endpoints.js#newco...
app/store/endpoints.js:56: db.services.after('reset', this.reset, this);
Good catch.  That's odd as I had done it but I think that change got lost. 
Oops.

https://codereview.appspot.com/8338043/diff/5001/app/store/endpoints.js#newco...
app/store/endpoints.js:89: this.endpointsMap = {};
Should this unbind too?

https://codereview.appspot.com/8338043/diff/5001/app/store/endpoints.js#newco...
app/store/endpoints.js:121: if (!Y.Lang.isValue(charm)) {
On 2013/04/03 22:18:27, jeff.pihach wrote:
> if (charm) { should suffice here.

if (!charm)...
Done

https://codereview.appspot.com/8338043/diff/5001/app/store/endpoints.js#newco...
app/store/endpoints.js:175: delete(this.endpointsMap[svcName]);
On 2013/04/03 22:18:27, jeff.pihach wrote:
> I think the official syntax for delete is `delete this.endpointsMap[svcName];`

Done.

https://codereview.appspot.com/8338043/diff/5001/test/test_endpoints.js
File test/test_endpoints.js (right):

https://codereview.appspot.com/8338043/diff/5001/test/test_endpoints.js#newco...
test/test_endpoints.js:34: app = new Y.juju.App({env: env});
On 2013/04/03 22:18:27, jeff.pihach wrote:
> these app instances should be destroyed in the afterEach.

Done.

https://codereview.appspot.com/8338043/diff/5001/test/test_endpoints.js#newco...
test/test_endpoints.js:143: controller = new EndpointsController();
On 2013/04/03 22:18:27, jeff.pihach wrote:
> this is not being destroyed in an afterEach

Done.

https://codereview.appspot.com/8338043/diff/5001/test/test_endpoints.js#newco...
test/test_endpoints.js:303: controller = app.endpointsController;
On 2013/04/03 22:18:27, jeff.pihach wrote:
> did you mean to create new instances here? If so they will then need to be
> destroyed in the afterEach :-)

Done.

https://codereview.appspot.com/8338043/diff/5001/test/test_environment_view.js
File test/test_environment_view.js (right):

https://codereview.appspot.com/8338043/diff/5001/test/test_environment_view.j...
test/test_environment_view.js:798: var endpointsController = new Y.Base();
Having a fully-fleshed controller is not important here, so I just created a
dummy object with the minimal required interface.

Renamed to fauxController

https://codereview.appspot.com/8338043/diff/5001/test/test_environment_view.j...
test/test_environment_view.js:842: assert.equal('hidy ho', endpointsController);
This test has lots of undestroyed objects.  Will clean up.

[jeff.pihach, 2013-04-04 14:21:57]

LGTM Thanks for making those fixes!

[bac, 2013-04-04 14:40:39]

*** Submitted:

Create EndpointsController.

Encapsulate the endpoints event handling.

R=jeff.pihach, bcsaller
CC=
https://codereview.appspot.com/8338043