Return to Revisions

Personally, I like comma separated variable definitions, but they ten to get messy when there are assignments or when they become multi-line. A suggestion I found on the net is to only use comma separated declarations if they are not assignments. If they are assignments, var them individually instead. looks cleaner and prevents stray ,.

//declarations only
var foo, bar, baz;

//assignments:
var foo = 1;
var bar = 2;
var baz = 3;

When declaring functions, I'd stick to using the "declaration notation" rather than the "expression notation" since:

• They contain lesser characters
• They don't look like variables at first sight
• Hoisting advantage
• Avoids the trailing ; (you'll know when you use linters)
• They are named which avoids issues with debuggers (better safe then sorry).

The notational difference is the following

//NOT THIS
var foo = function(){/*...*/};

//BUT THIS!
function foo(){/*...*/}

As for your nesting functions, they look fine but I suggest you indent properly.

There is a minor overhead in object property access, and you can cache them for shorter access. However, it's a trade-off between readability at times.

You can also assign the function name as callback directly rather than create a function that only calls another function.

var shareOnFacebook = scope.sharingActions.shareOnFacebook;


function loginSharePublishNavigate() {
  facebookActions.login().done(sharePublishNavigate);
}

function shareNavigate() {
  shareOnFacebook().done(app.router.navigateToStories);
}

function sharePublishNavigate() {
  shareOnFacebook().done(function () {
    scope.model.publish().done(app.router.navigateToStories);
  });
}

For boolean-containing variables, I suggest to be uniform in the name. I usually name them in the true state and prefixed with is or has. So instead of using noViews, I suggest you use hasViews. That way, the conditions will not be garbled in meaning:

//instead of
if(!noViews && hasEditPermissions){/*...*/}

//use this
if(hasViews && hasEditPermissions){/*...*/}

As for the logic, I can't quite figure it out. Here are the possible combinations of the 3 variables used.

view edit login
0    0    0     = loginSharePublishNavigate
0    0    1     = shareNavigate
0    1    0     = sharePublishNavigate
0    1    1     = sharePublishNavigate
1    0    0     = ?
1    0    1     = ?
1    1    0     = shareNavigate
1    1    1     = shareNavigate

Let's say we factor out loggedIn as our first check and should do loginSharePublishNavigate,

view edit login
/*
0    0    0     = loginSharePublishNavigate
0    1    0     = loginSharePublishNavigate
1    1    0     = loginSharePublishNavigate
1    0    0     = loginSharePublishNavigate
*/
1    0    1     = ?
0    1    1     = sharePublishNavigate
0    0    1     = shareNavigate
1    1    1     = shareNavigate

Still, can't figure it out.