0
\$\begingroup\$

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?

\$\endgroup\$
5
  • 5
    \$\begingroup\$ First thing: use correct indentation! \$\endgroup\$ May 27, 2013 at 12:10
  • \$\begingroup\$ Any reason for sharing something, and then publishing it? Seems exactly backwards to me. \$\endgroup\$
    – Flambino
    May 27, 2013 at 12:22
  • \$\begingroup\$ By the way, you can probably use .then to your advantage, provided you're using jQuery 1.8+: scope.sharingActions.shareOnFacebook().then(scope.model.publish).then(app.router.navigateToStories); \$\endgroup\$
    – Flambino
    May 27, 2013 at 12:38
  • \$\begingroup\$ Why do you declare those functions locally instead of in an object of their own? \$\endgroup\$ May 27, 2013 at 13:12
  • \$\begingroup\$ Agree @Flambino, and I would put each .then on a new line, I know that isn't possible in comments. \$\endgroup\$
    – Dan Ross
    May 28, 2013 at 4:56

3 Answers 3

2
\$\begingroup\$

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.

\$\endgroup\$
2
\$\begingroup\$

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.

\$\endgroup\$
1
\$\begingroup\$

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

\$\endgroup\$
2
  • \$\begingroup\$ Why did you made: rootPromise.then(login).then(share).then(publish).then(navigate); instead of: login().then(share).then(publish).then(navigate); \$\endgroup\$
    – Naor
    May 29, 2013 at 13:11
  • \$\begingroup\$ I was wondering if you might ask. As I said, rootPromise isn't strictly necessary and yes, login().... is the other way to proceed but in that case login() would need to return a promise under both conditions - isLoggedIn and !isLoggedIn - otherwise login().then() would throw an error. Using a ready-resolved "rootPromise" or "starterPromose" is quite a common technique when building .then() chains. \$\endgroup\$ May 29, 2013 at 16:25

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service and acknowledge you have read our privacy policy.

Not the answer you're looking for? Browse other questions tagged or ask your own question.