Support for sharing stories on Facebook

Question

Is there a way to write this code block cleaner and more readable?

 var shareNavigate = function () {
     scope.sharingActions.shareOnFacebook().done(function () {
             app.router.navigateToStories();
         });
 },
     sharePublishNavigate = function () {
         scope.sharingActions.shareOnFacebook().done(function () {
                 scope.model.publish().done(function () {
                         app.router.navigateToStories();
                     });
             });
     },
     loginSharePublishNavigate = function () {
         facebookActions.login().done(function () {
                 sharePublishNavigate();
             });
     };
 if (!noViews && hasEditPermissions) {
     shareNavigate();
 } else if (noViews) {
     if (hasEditPermissions) {
         sharePublishNavigate();
     } else if (!isLoggeedIn) {
         loginSharePublishNavigate();
     } else {
         shareNavigate();
     }
 }

First I define functions and then in each case I call the relevant function. I am using jQuery deferreds and since I need operation to done, I have many nested methods. Is there any way to beautify this code?

Answer 1

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 "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).

The notational difference is the following

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

//declaration - 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 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 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.

Answer 2

I'd prefer to rewrite this

 if (!noViews && hasEditPermissions) {
     shareNavigate();
 } else if (noViews) {
     if (hasEditPermissions) {
         sharePublishNavigate();
     } else if (!isLoggeedIn) {
         loginSharePublishNavigate();
     } else {
         shareNavigate();
     }
 }

To this:

 if (hasEditPermissions) {
     if (noViews) {
         sharePublishNavigate();
     } else {
         shareNavigate();
     }
 } else {
     if(loggeedIn) {
         shareNavigate();
     } else {
         loginSharePublishNavigate();
     }
 }

The !noViews is redundant and it is usually confusing if you start an if-else expression with a negation. Besides that, try avoiding else if in favor of nested expressions. You should also try to make the boolean expressions functionally independent - if you have checked noViews somewhere, don't check it again.

I am not sure if the truth table for the two blocks are identical. I'll leave that up to you to verify.

Answer 3

You would possibly be better off with login, share, publish and navigate as promise-returning worker functions:

var login = function() { return isLoggedIn ? null : facebookActions.login(); }
var share = function() { return scope.sharingActions.shareOnFacebook(); };
var publish = function() { return scope.model.publish() };
var navigate = function() { return app.router.navigateToStories() };

Now you can sequence these workers with .then() as follows (or similar) :

var rootPromise = $.Deferred().resolve().promise();

if (noViews) {
    if(hasEditPermissions || !isLoggedIn) {
        rootPromise.then(login).then(share).then(publish).then(navigate);
    }
    else {
        rootPromise.then(share).then(navigate);
    }
}
else if(hasEditPermissions) {
    rootPromise.then(share).then(navigate);
}

Notes:

• rootPromise isn't strictly necessary but keeps rest of the code tidier, particularly login().

• login() returns null if already logged in, allowing the calling logic to be slightly simpler.

• The whole thing may simplify further depending in part on the relationship between hasEditPermissions and isLoggedIn (See other answers for suggestions).