refactored manageUnitsMixin and added tests

refactored manageUnitsMixin and added tests

Full details:

- refactored manageUnitsMixin and added tests for it
- added documentation for manageUnitsMixin
- reconsidered our object literal style, updated the style guide to reflect my
  current notions, and used the style in the code as a proof of concept
- captured current naming practice in the style guide (camelCase everywhere)
- tweaked all of the tests so the test container element is "display: none"
  when possible and "visibility: hidden" otherwise
- transmuted an "apply" to a slightly simpler "call"

https://code.launchpad.net/~benji/juju-gui/bug-1074336/+merge/135261

(do not edit description out of merge proposal)

[benji, 2012-11-20 22:03:58]

Please take a look.

[frankban, 2012-11-21 11:26:13]

The code is easy to follow and everything looks very readable Benji, thank you!

I have some comments below, especially the problem of changing the unit count
using the input box. Otherwise this branch looks good and the tests pass.

https://codereview.appspot.com/6845065/diff/1/app/views/service.js
File app/views/service.js (right):

https://codereview.appspot.com/6845065/diff/1/app/views/service.js#newcode48
app/views/service.js:48: * @param {Number} keyCode The key that was pressed.
The parameter fieldValue should be documented here.

https://codereview.appspot.com/6845065/diff/1/app/views/service.js#newcode56
app/views/service.js:56: var numberRegex = /^\d+$/;
Thanks for defining the regex so that humans can understand what's going on.

https://codereview.appspot.com/6845065/diff/1/app/views/service.js#newcode77
app/views/service.js:77: ev.halt(this._handleKeyPress(ev.keyCode,
field.get('value')));
While trying this branch, I was not able to change the unit count using the
input field. It seems that this line always stops the event propagation, and
that the boolean returned by _handleKeyPress is only used as the `immediate` arg
of ev.halt. However, didn't investigated, so, maybe I am wrong.

https://codereview.appspot.com/6845065/diff/1/app/views/service.js#newcode92
app/views/service.js:92: db = this.get('db');
db does not seem used by this method.

https://codereview.appspot.com/6845065/diff/1/app/views/service.js#newcode111
app/views/service.js:111: * @param {Function} getUnits A function that returns
all the units for a
These callable parameters are not passed to the method: they seem to be still
called as env methods.

https://codereview.appspot.com/6845065/diff/1/docs/style-guide.rst
File docs/style-guide.rst (right):

https://codereview.appspot.com/6845065/diff/1/docs/style-guide.rst#newcode18
docs/style-guide.rst:18: Variable declaration
+1 Let's see what the other guys of the squad think about this.

https://codereview.appspot.com/6845065/diff/1/docs/style-guide.rst#newcode50
docs/style-guide.rst:50: Naming
Cool.

https://codereview.appspot.com/6845065/diff/1/docs/style-guide.rst#newcode62
docs/style-guide.rst:62: - object literals have their opening brace on the same
line as the
Looks good to me.

https://codereview.appspot.com/6845065/diff/1/docs/style-guide.rst#newcode63
docs/style-guide.rst:63: equals sign (e.g., "var foo = {"
Missing closing bracket.

https://codereview.appspot.com/6845065/diff/1/docs/style-guide.rst#newcode87
docs/style-guide.rst:87: int_bad: 'nope!'},
This example seems to contradict the rules above.
There is multiple var definition (separated by commas), and the old object
literal style.

https://codereview.appspot.com/6845065/diff/1/test/test_app.js
File test/test_app.js (right):

https://codereview.appspot.com/6845065/diff/1/test/test_app.js#newcode135
test/test_app.js:135: container = Y.Node.create('<div/>')
So much better!

https://codereview.appspot.com/6845065/diff/1/test/test_app.js#newcode138
test/test_app.js:138: container.hide();
Isn't the container already hidden by the line above?

https://codereview.appspot.com/6845065/diff/1/test/test_manageUnitsMixin.js
File test/test_manageUnitsMixin.js (right):

https://codereview.appspot.com/6845065/diff/1/test/test_manageUnitsMixin.js#n...
test/test_manageUnitsMixin.js:20: after(function(done)  {
Is this required by the testing framework?

https://codereview.appspot.com/6845065/diff/1/test/test_manageUnitsMixin.js#n...
test/test_manageUnitsMixin.js:111: it('should reset the unit count a non-number
is entered', function() {
Missing 'when' in the test description.

https://codereview.appspot.com/6845065/diff/1/undocumented
File undocumented (left):

https://codereview.appspot.com/6845065/diff/1/undocumented#oldcode154
undocumented:154: app/views/service.js _modifyUnits
Thank you from the future!

[thiago, 2012-11-21 13:42:24]

Hi Benji, thanks for the improvement.
I love the multiple "var" declarations... and Douglas Crockford agree with us.
:O)

The feature seems to be broken. I have a comment for that.

[]s,
Thiago.

https://codereview.appspot.com/6845065/diff/1/app/views/service.js
File app/views/service.js (right):

https://codereview.appspot.com/6845065/diff/1/app/views/service.js#newcode58
app/views/service.js:58: this._modifyUnits(parseInt(fieldValue, 10));
I've found a YUI utility for it.

http://yuilibrary.com/yui/docs/api/classes/Number.html

"parse"

https://codereview.appspot.com/6845065/diff/1/app/views/service.js#newcode76
app/views/service.js:76: var field = this._getInputBox();
This feature is not working. I cannot change the field value. If you change this
method to...

    handleKeyEvent: function(ev) {
      if (ev.keyCode !== ESC && ev.keyCode !== ENTER) {
        return;
      }
      
      var field = this._getInputBox();
      ev.halt(true);
      this._handleKeyPress(ev.keyCode, field.get('value'));
    },

... it starts working again. I think it is something related to the
"field.get('value')" method call.

https://codereview.appspot.com/6845065/diff/1/app/views/service.js#newcode91
app/views/service.js:91: env = this.get('env'),
env never used?

https://codereview.appspot.com/6845065/diff/1/docs/style-guide.rst
File docs/style-guide.rst (right):

https://codereview.appspot.com/6845065/diff/1/docs/style-guide.rst#newcode26
docs/style-guide.rst:26: var a = 1;
A big +1 for it!
Thanks!

https://codereview.appspot.com/6845065/diff/1/test/test_app.js
File test/test_app.js (right):

https://codereview.appspot.com/6845065/diff/1/test/test_app.js#newcode138
test/test_app.js:138: container.hide();
In other test files we simply remove the elements from the DOM. Probably you
could do the same.

https://codereview.appspot.com/6845065/diff/1/test/test_app.js#newcode199
test/test_app.js:199: container = Y.Node.create('<div/>')
Why? We already remove it from the dom after every test.

https://codereview.appspot.com/6845065/diff/1/test/test_application_notificat...
File test/test_application_notifications.js (right):

https://codereview.appspot.com/6845065/diff/1/test/test_application_notificat...
test/test_application_notifications.js:42: applicationContainer =
Y.Node.create('<div/>').hide();
Why? We already remove it from the dom after every test.

https://codereview.appspot.com/6845065/diff/1/test/test_charm_collection_view.js
File test/test_charm_collection_view.js (right):

https://codereview.appspot.com/6845065/diff/1/test/test_charm_collection_view...
test/test_charm_collection_view.js:108: var container =
Y.Node.create('<div/>').hide();
We already destroy it in this method. I don't think we need to hide it.

https://codereview.appspot.com/6845065/diff/1/test/test_charm_view.js
File test/test_charm_view.js (right):

https://codereview.appspot.com/6845065/diff/1/test/test_charm_view.js#newcode57
test/test_charm_view.js:57: container = Y.Node.create('<div/>').hide();
ditto

https://codereview.appspot.com/6845065/diff/1/test/test_environment_view.js
File test/test_environment_view.js (right):

https://codereview.appspot.com/6845065/diff/1/test/test_environment_view.js#n...
test/test_environment_view.js:106: container = Y.Node.create('<div/>')
We already destroy it.

https://codereview.appspot.com/6845065/diff/1/test/test_manageUnitsMixin.js
File test/test_manageUnitsMixin.js (right):

https://codereview.appspot.com/6845065/diff/1/test/test_manageUnitsMixin.js#n...
test/test_manageUnitsMixin.js:3: (function() {
Why do you wrap everything inside this function? "describe" already does that.

https://codereview.appspot.com/6845065/diff/1/test/test_manageUnitsMixin.js#n...
test/test_manageUnitsMixin.js:21: done();
You dont need to call it.
In fact, you should avoid the use of this magic "done" method.

https://codereview.appspot.com/6845065/diff/1/test/test_manageUnitsMixin.js#n...
test/test_manageUnitsMixin.js:25: container = Y.Node.create('<div
id=\"test-container\"/>').hide();
Avoid "done". This is not a async execution.

https://codereview.appspot.com/6845065/diff/1/test/test_manageUnitsMixin.js#n...
test/test_manageUnitsMixin.js:31: container.remove(true);
If you keep "done", you risk of having a call to an "it" method before the
previous "it" execution calls the "afterEach" method.

https://codereview.appspot.com/6845065/diff/1/test/test_manageUnitsMixin.js#n...
test/test_manageUnitsMixin.js:36: var fauxField = {};
I like the var declaration! Thnaks!

https://codereview.appspot.com/6845065/diff/1/test/test_service_view.js
File test/test_service_view.js (right):

https://codereview.appspot.com/6845065/diff/1/test/test_service_view.js#newco...
test/test_service_view.js:26: container = Y.Node.create('<div/>').hide();
ditto

[benji, 2012-11-21 17:56:41]

Thanks to Francesco and Thiago for the good reviews.  I have addressed (almost)
everything brought up.  The only unaddressed things were minor misunderstandings
(I hope).

https://codereview.appspot.com/6845065/diff/1/app/views/service.js
File app/views/service.js (right):

https://codereview.appspot.com/6845065/diff/1/app/views/service.js#newcode48
app/views/service.js:48: * @param {Number} keyCode The key that was pressed.
On 2012/11/21 11:26:14, francesco.banconi wrote:
> The parameter fieldValue should be documented here.

Good catch.  I have fixed this and added a slack card to extend the yuidoc
linter to check for this.

https://codereview.appspot.com/6845065/diff/1/app/views/service.js#newcode56
app/views/service.js:56: var numberRegex = /^\d+$/;
On 2012/11/21 11:26:14, francesco.banconi wrote:
> Thanks for defining the regex so that humans can understand what's going on.

I really wanted to this:

var isNumber = /^\d+$/.test;
if (isNumber(fieldValue)) {
...

But because of Javascript's infernal binding rules that isn't possible without
an intermediate variable, making the current version preferable.

https://codereview.appspot.com/6845065/diff/1/app/views/service.js#newcode58
app/views/service.js:58: this._modifyUnits(parseInt(fieldValue, 10));
On 2012/11/21 13:42:24, thiago wrote:
> I've found a YUI utility for it.
> 
> http://yuilibrary.com/yui/docs/api/classes/Number.html
> 
> "parse"

What are the benefits of using YUI for this?

https://codereview.appspot.com/6845065/diff/1/app/views/service.js#newcode76
app/views/service.js:76: var field = this._getInputBox();
On 2012/11/21 13:42:24, thiago wrote:
> This feature is not working. I cannot change the field value. If you change
this
> method to...
> 
>     handleKeyEvent: function(ev) {
>       if (ev.keyCode !== ESC && ev.keyCode !== ENTER) {
>         return;
>       }
>       
>       var field = this._getInputBox();
>       ev.halt(true);
>       this._handleKeyPress(ev.keyCode, field.get('value'));
>     },
> 
> ... it starts working again. I think it is something related to the
> "field.get('value')" method call. 

I see what happened, I misunderstood the meaning of the argument to halt().  I
have fixed this.

https://codereview.appspot.com/6845065/diff/1/app/views/service.js#newcode91
app/views/service.js:91: env = this.get('env'),
On 2012/11/21 13:42:24, thiago wrote:
> env never used?

Nope, fixed.

https://codereview.appspot.com/6845065/diff/1/app/views/service.js#newcode92
app/views/service.js:92: db = this.get('db');
On 2012/11/21 11:26:14, francesco.banconi wrote:
> db does not seem used by this method.

Fixed.

https://codereview.appspot.com/6845065/diff/1/app/views/service.js#newcode111
app/views/service.js:111: * @param {Function} getUnits A function that returns
all the units for a
On 2012/11/21 11:26:14, francesco.banconi wrote:
> These callable parameters are not passed to the method: they seem to be still
> called as env methods.

Indeed.  I got excited about removing some this.get()s but Javascript binding
rules thwarted me.  Fixed.

https://codereview.appspot.com/6845065/diff/1/docs/style-guide.rst
File docs/style-guide.rst (right):

https://codereview.appspot.com/6845065/diff/1/docs/style-guide.rst#newcode63
docs/style-guide.rst:63: equals sign (e.g., "var foo = {"
On 2012/11/21 11:26:14, francesco.banconi wrote:
> Missing closing bracket.

Too many brackets I guess. :)  Fixed.

https://codereview.appspot.com/6845065/diff/1/docs/style-guide.rst#newcode87
docs/style-guide.rst:87: int_bad: 'nope!'},
On 2012/11/21 11:26:14, francesco.banconi wrote:
> This example seems to contradict the rules above.
> There is multiple var definition (separated by commas), and the old object
> literal style.

Indeed, it was left from the old style.  I fixed it.

I'm still not happy that object literal brace indents are different from
function brace indents, but reconciling them would seem to be uncommon,
style-wise.

https://codereview.appspot.com/6845065/diff/1/test/test_app.js
File test/test_app.js (right):

https://codereview.appspot.com/6845065/diff/1/test/test_app.js#newcode138
test/test_app.js:138: container.hide();
On 2012/11/21 11:26:14, francesco.banconi wrote:
> Isn't the container already hidden by the line above?

Heh, yep.  That was left over from an earlier revision.  Fixed.

https://codereview.appspot.com/6845065/diff/1/test/test_app.js#newcode138
test/test_app.js:138: container.hide();
On 2012/11/21 13:42:24, thiago wrote:
> In other test files we simply remove the elements from the DOM. Probably you
> could do the same.

I don't see that in the other tests.  Maybe I am missing something.

https://codereview.appspot.com/6845065/diff/1/test/test_app.js#newcode199
test/test_app.js:199: container = Y.Node.create('<div/>')
On 2012/11/21 13:42:24, thiago wrote:
> Why? We already remove it from the dom after every test.

I don't understand the question.  If it is "Why are we hiding the test
container?" then the reason is that it stops the test page flicker.  I may be
the only one bothered by it, but the cure seems cheap enough.

https://codereview.appspot.com/6845065/diff/1/test/test_application_notificat...
File test/test_application_notifications.js (right):

https://codereview.appspot.com/6845065/diff/1/test/test_application_notificat...
test/test_application_notifications.js:42: applicationContainer =
Y.Node.create('<div/>').hide();
On 2012/11/21 13:42:24, thiago wrote:
> Why? We already remove it from the dom after every test.

See earlier response.

https://codereview.appspot.com/6845065/diff/1/test/test_charm_collection_view.js
File test/test_charm_collection_view.js (right):

https://codereview.appspot.com/6845065/diff/1/test/test_charm_collection_view...
test/test_charm_collection_view.js:108: var container =
Y.Node.create('<div/>').hide();
On 2012/11/21 13:42:24, thiago wrote:
> We already destroy it in this method. I don't think we need to hide it.

See earlier response.

https://codereview.appspot.com/6845065/diff/1/test/test_manageUnitsMixin.js
File test/test_manageUnitsMixin.js (right):

https://codereview.appspot.com/6845065/diff/1/test/test_manageUnitsMixin.js#n...
test/test_manageUnitsMixin.js:3: (function() {
On 2012/11/21 13:42:24, thiago wrote:
> Why do you wrap everything inside this function? "describe" already does that.

I cribbed this from another test module.  I don't know why they do it that way. 
I am more than happy to remove the outer function if we are reasonably sure it
isn't preforming an important function.

This is one of the reasons less is more: unnecessary code tends to perpetuate
itself.

https://codereview.appspot.com/6845065/diff/1/test/test_manageUnitsMixin.js#n...
test/test_manageUnitsMixin.js:20: after(function(done)  {
On 2012/11/21 11:26:14, francesco.banconi wrote:
> Is this required by the testing framework?

Good catch.  This was left from using another test module as a starting point. 
Fixed.

https://codereview.appspot.com/6845065/diff/1/test/test_manageUnitsMixin.js#n...
test/test_manageUnitsMixin.js:21: done();
On 2012/11/21 13:42:24, thiago wrote:
> You dont need to call it.
> In fact, you should avoid the use of this magic "done" method.

Indeed!  Fixed.

https://codereview.appspot.com/6845065/diff/1/test/test_manageUnitsMixin.js#n...
test/test_manageUnitsMixin.js:25: container = Y.Node.create('<div
id=\"test-container\"/>').hide();
On 2012/11/21 13:42:24, thiago wrote:
> Avoid "done". This is not a async execution.  

Done.

https://codereview.appspot.com/6845065/diff/1/test/test_manageUnitsMixin.js#n...
test/test_manageUnitsMixin.js:31: container.remove(true);
On 2012/11/21 13:42:24, thiago wrote:
> If you keep "done", you risk of having a call to an "it" method before the
> previous "it" execution calls the "afterEach" method.

Done.

https://codereview.appspot.com/6845065/diff/1/test/test_manageUnitsMixin.js#n...
test/test_manageUnitsMixin.js:111: it('should reset the unit count a non-number
is entered', function() {
On 2012/11/21 11:26:14, francesco.banconi wrote:
> Missing 'when' in the test description.

I added the missing "when". I really wanted to use more words, but that forced a
line break, which would made the linter require insane indentation for the body
of the function. <sigh>