Skip to main content
added 137 characters in body
Source Link
Joseph
  • 25.2k
  • 2
  • 25
  • 37

Still, can't figure it out. If I do some K-map on this, I might get the right combinations, but end up with a more complex condition. Anyways, a generic suggestion is you do something like a "filter conditions"filtering rather than nesting conditions and doing &&. Something like:

if(!loggedIn){
  //now in here, it's executing `!loggedIn`
  //not logged in
}
//ok, so we are logged in, what next?
else if(!hasPermissions){
  //now in here, it's like executing `loggedIn && !hasPermissions`
  //no permissions
}
//ok, we are now logged in and have permissions, what next?
else if(!anotherCheck){
  //now in here, it's like executing `loggedIn && hasPermissions`hasPermissions && !anotherCheck`
} 
//...and so on...
else {
  //now in here, it's like executing `loggedIn && hasPermissions && anotherCheck`
  //ok, every check is true by this block
}

We avoided nesting. But this only works in linear conditions. If there are more complex conditions, this won't work. Also, it passes through more checks when all conditions are true which means more overhead. But it's a trade-off for readable code.

Still, can't figure it out. If I do some K-map on this, I might get the right combinations, but end up with a more complex condition. Anyways, a generic suggestion is you do something like a "filter conditions" rather than nesting conditions and doing &&. Something like:

if(!loggedIn){
  //not logged in
}
//ok, so we are logged in, what next?
else if(!hasPermissions){
  //no permissions
}
//ok, we are now logged in and have permissions, what next?
else if(anotherCheck){
  //now in here, it's like executing `loggedIn && hasPermissions`
} 
//...and so on...
else {
  //now in here, it's like executing `loggedIn && hasPermissions && anotherCheck`
  //ok, every check is true by this block
}

We avoided nesting. But this only works in linear conditions. If there are more complex conditions, this won't work.

Still, can't figure it out. If I do some K-map on this, I might get the right combinations, but end up with a more complex condition. Anyways, a generic suggestion is you do filtering rather than nesting:

if(!loggedIn){
  //now in here, it's executing `!loggedIn`
  //not logged in
}
//ok, so we are logged in, what next?
else if(!hasPermissions){
  //now in here, it's like executing `loggedIn && !hasPermissions`
  //no permissions
}
//ok, we are now logged in and have permissions, what next?
else if(!anotherCheck){
  //now in here, it's like executing `loggedIn && hasPermissions && !anotherCheck`
} 
//...and so on...
else {
  //now in here, it's like executing `loggedIn && hasPermissions && anotherCheck`
  //ok, every check is true by this block
}

We avoided nesting. But this only works in linear conditions. If there are more complex conditions, this won't work. Also, it passes through more checks when all conditions are true which means more overhead. But it's a trade-off for readable code.

added 667 characters in body
Source Link
Joseph
  • 25.2k
  • 2
  • 25
  • 37

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

  • They contain lesser characters
  • They don't look like variables at first sight
  • Hoisting advantage (the compiler pulls them up the scope so they look like they are declared on top, regardless of where you place them in the code)
  • Avoids the trailing ; (you'll know when you use linters)
  • They are named which avoids issues with debuggers (better safe then sorry).
//expression - NOT THIS
var foo = function(){/*...*/};

//BUTdeclaration - THIS!
function foo(){/*...*/}

Let's say we factor out loggedIn as our first check and should do loginSharePublishNavigate, when not logged in.

view edit login
/*
0    0    0     = loginSharePublishNavigate
0    1    0     = loginSharePublishNavigate
1    1    0     = loginSharePublishNavigate
1    0    0     = loginSharePublishNavigate
*/
1    0    1     = ? <- what handles this???
0    1    1     = sharePublishNavigate
0    0    1     = shareNavigate
1    1    1     = shareNavigate

Still, can't figure it out. If I do some K-map on this, I might get the right combinations, but end up with a more complex condition. Anyways, a generic suggestion is you do something like a "filter conditions" rather than nesting conditions and doing &&. Something like:

if(!loggedIn){
  //not logged in
}
//ok, so we are logged in, what next?
else if(!hasPermissions){
  //no permissions
}
//ok, we are now logged in and have permissions, what next?
else if(anotherCheck){
  //now in here, it's like executing `loggedIn && hasPermissions`
} 
//...and so on...
else {
  //now in here, it's like executing `loggedIn && hasPermissions && anotherCheck`
  //ok, every check is true by this block
}

We avoided nesting. But this only works in linear conditions. If there are more complex conditions, this won't work.

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).
//NOT THIS
var foo = function(){/*...*/};

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

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.

When declaring functions, I'd stick to using "function declarations" rather than the "function expressions" since:

  • They contain lesser characters
  • They don't look like variables at first sight
  • Hoisting advantage (the compiler pulls them up the scope so they look like they are declared on top, regardless of where you place them in the code)
  • Avoids the trailing ; (you'll know when you use linters)
  • They are named which avoids issues with debuggers (better safe then sorry).
//expression - NOT THIS
var foo = function(){/*...*/};

//declaration - THIS!
function foo(){/*...*/}

Let's say we factor out loggedIn as our first check and should do loginSharePublishNavigate when not logged in.

view edit login
/*
0    0    0     = loginSharePublishNavigate
0    1    0     = loginSharePublishNavigate
1    1    0     = loginSharePublishNavigate
1    0    0     = loginSharePublishNavigate
*/
1    0    1     = ? <- what handles this???
0    1    1     = sharePublishNavigate
0    0    1     = shareNavigate
1    1    1     = shareNavigate

Still, can't figure it out. If I do some K-map on this, I might get the right combinations, but end up with a more complex condition. Anyways, a generic suggestion is you do something like a "filter conditions" rather than nesting conditions and doing &&. Something like:

if(!loggedIn){
  //not logged in
}
//ok, so we are logged in, what next?
else if(!hasPermissions){
  //no permissions
}
//ok, we are now logged in and have permissions, what next?
else if(anotherCheck){
  //now in here, it's like executing `loggedIn && hasPermissions`
} 
//...and so on...
else {
  //now in here, it's like executing `loggedIn && hasPermissions && anotherCheck`
  //ok, every check is true by this block
}

We avoided nesting. But this only works in linear conditions. If there are more complex conditions, this won't work.

Source Link
Joseph
  • 25.2k
  • 2
  • 25
  • 37

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.