Code review comment for lp:~bcsaller/juju-gui/namespace-routing

Revision history for this message
Jeff Pihach (hatch) wrote :

This looks great! I just have a couple small comments :-)

https://codereview.appspot.com/7327045/diff/16001/app/assets/javascripts/routing.js
File app/assets/javascripts/routing.js (right):

https://codereview.appspot.com/7327045/diff/16001/app/assets/javascripts/routing.js#newcode5
app/assets/javascripts/routing.js:5: function _trim(s, char, leading,
trailing) {
This feels like a generic utility method which could be split out into a
separate utility method module and hung off Y.Lang so that it's
available application wide.

As a generic utility method it would probably be best solved using a
regex. Ln 5-18 could be replaced with:

** WARNING - incoming untested code **

Y.Lang.trimString = function(str, start, end) {

   var trimRegex = new RegExp("^[" + start + "]+|[" + end + "]+$","g");
   return str.replace(trimRegex, "");

}

https://codereview.appspot.com/7327045/diff/16001/app/assets/javascripts/routing.js#newcode90
app/assets/javascripts/routing.js:90: console.log('URL namespace without
path');
If you're creating logs for debugging it's best to use Y.log() because
it can then be filtered in or out during debugging instead of always
logging to the console.

https://codereview.appspot.com/7327045/diff/16001/app/assets/javascripts/routing.js#newcode96
app/assets/javascripts/routing.js:96: console.log('URL has more than one
refernce to same namespace');
s/refernce/reference :-) also see above comment about Y.log()

https://codereview.appspot.com/7327045/diff/16001/app/assets/javascripts/routing.js#newcode113
app/assets/javascripts/routing.js:113: var u = '/';
u is url? Please don't manually minify - use meaningful variable names
</pet peeve> :-)

https://codereview.appspot.com/7327045/

« Back to merge proposal