Avoiding callbacks, and nest if statements

Question

How could I avoid this mess??, I'd like to shrink the code somehow, about the callbacks I'll most like use the await/promise features of javascript, but for the nesting if's I really don't know what to do. Any tips about refactoring this mess would be great Thank you.

router.delete("/user/:username", passport.authenticate("jwt", { session: false }), (req, res) => {
  var { username = null } = req.params;
  if (username != null) {
    User.getUserById(req.user.id, (err, destroyerUser) => {
      if (err) {
        res.json({ success: false, msg: "User not found" });
      } else {
        if (destroyerUser) {
          if (destroyerUser.role == "Admin" || destroyerUser.role == "Moderator") {
            User.getUserByUsername(username, (err, user) => {
              if (!err) {
                if (user) {
                  if (user._id.toString() != destroyerUser._id.toString()) {
                    if (destroyerUser.role == "Admin") {
                      Cyclic.deleteOneById({ _id: user._id }, (err) => {
                        if (err) {
                          res.json({ success: false, msg: "Error deleting user" });
                        } else {
                          res.json({ success: true, msg: "Successfully deleted user" });
                        }
                      })
                    } else if (destroyerUser.role == "Moderator") {
                      if (user.role == "Moderator" || user.role == "Admin") {
                        res.json({ success: false, msg: "You don't have sufficient permissions" })
                      } else {
                        Cyclic.deleteOneById({ _id: user._id }, (err) => {
                          if (err) {
                            res.json({ success: false, msg: "Error deleting user" });
                          } else {
                            res.json({ success: true, msg: "Successfully deleted user" });
                          }
                        })
                      }
                    }
                  } else {
                    res.json({ success: false, msg: "You can't delete yourself" });
                  }
                } else {
                  res.json({ success: false, msg: "User not found" });
                }
              } else {
                res.json({ success: false, msg: "Error finding user" });
              }
            })
          } else {
            res.json({ success: false, msg: "You don't have sufficient permissions" })
          }
        } else {
          res.json({ success: false, msg: "Destroyer user not found" });
        }
      }
    });
  } else {
    res.json({ success: false, msg: "Username is not valid" });
  }
})

Answer 1

There are a few things you can do with nested sideways pyramids. One technique that's pretty universal is to handle errors first and then do your normal code. So, let's say you have:

if (everythingFine) {
  //do normal operation
} else {
  //handle error
}

This is OK but you really run into problems as soon as you get

if (everythingFine) {
  //do operation
  if (operationSuccessful) {
    //do next step
  } else {
    //handle error with operation
  }
} else {
  //handle error
}

You immediately start stacking more and more blocks. What's more, the logic for success is usually a lot big, since you need to do more stuff later. And the error handling logic might be a single line. So, if you want to know what's happening, on error, you need to scroll a lot.

To avoid this, you can exit early. The steps to convert your code are the same:

1. Flip the condition of the if that checks for success to instead check for errors
2. Swap the if block and the else block.
3. Add a return to the if.
4. Remove the else block and flatten it.

And you get the following

if (!everythingFine) {
  //handle error
  return;
}

//do operation
if (!operationSuccessful) {
  //handle error with operation
  return;    
}

//do next step

Now you remove the nesting and drop the else blocks, since you are going to just return and exit the function. If your error handling is a throw statement that already exists the execution, so you don't need return.

The next thing is that you have some duplicate logic here:

if (destroyerUser.role == "Admin") {
    Cyclic.deleteOneById({ _id: user._id }, (err) => {
        if (err) {
            res.json({ success: false, msg: "Error deleting user" });
        } else {
            res.json({ success: true, msg: "Successfully deleted user" });
        }
    })
} else if (destroyerUser.role == "Moderator") {
    if (user.role == "Moderator" || user.role == "Admin") {
        res.json({ success: false, msg: "You don't have sufficient permissions" })
    } else {
        Cyclic.deleteOneById({ _id: user._id }, (err) => {
            if (err) {
                res.json({ success: false, msg: "Error deleting user" });
            } else {
                res.json({ success: true, msg: "Successfully deleted user" });
            }
        })
    }
}

Whether destroyerUser is Admin or Moderator you call the same deleteByOne in either case. And you also return the same success message in both cases. So, you have six different branches in the code whereas you only have three outcomes:

+----------------+-----------+-----------------+-----------------------------------------+
| destroyer role | user role | deletion result |            operation result             |
+----------------+-----------+-----------------+-----------------------------------------+
| Moderator      | Moderator | N/A             | "You don't have sufficient permissions" |
| Moderator      | Admin     | N/A             | "You don't have sufficient permissions" |
| Moderator      | <other>   | success         | "Successfully deleted user"             |
| Moderator      | <other>   | error           | "Error deleting user"                   |
| Admin          | <any>     | success         | "Successfully deleted user"             |
| Admin          | <any>     | error           | "Error deleting user"                   |
+----------------+-----------+-----------------+-----------------------------------------+

Instead you can just do the check if a Moderator is doing the deletion first, and if they do have sufficient permissions, they are an Admin, then just do the deletion once:

if (destroyerUser.role == "Moderator" && (user.role == "Moderator" || user.role == "Admin")) {
    res.json({ success: false, msg: "You don't have sufficient permissions" })
    return;
}

Cyclic.deleteOneById({ _id: user._id }, (err) => {
    if (err) {
        res.json({ success: false, msg: "Error deleting user" });
        return;
    }
    res.json({ success: true, msg: "Successfully deleted user" });
})

So, if we apply these two thing:

• invert the logic and exit early
• remove the duplicate code

Then your code looks like this:

router.delete("/user/:username", passport.authenticate("jwt", { session: false }), (req, res) => {
    var { username = null } = req.params;
    if (username == null) {
        res.json({ success: false, msg: "Username is not valid" });
        return;
    }

    User.getUserById(req.user.id, (err, destroyerUser) => {
        if (err) {
            res.json({ success: false, msg: "User not found" });
            return;
        }
        if (!destroyerUser) {
            res.json({ success: false, msg: "Destroyer user not found" });
            return;
        }
        if (destroyerUser.role !== "Admin" && destroyerUser.role !== "Moderator") {
            res.json({ success: false, msg: "You don't have sufficient permissions" })
            return;
        }

        User.getUserByUsername(username, (err, user) => {
            if (err) {
                res.json({ success: false, msg: "Error finding user" });
                return;
            }
            if (!user) {
                res.json({ success: false, msg: "User not found" });
                return;
            }
            if (user._id.toString() === destroyerUser._id.toString()) {
                res.json({ success: false, msg: "You can't delete yourself" });
                return;
            }
            if (destroyerUser.role == "Moderator" && (user.role == "Moderator" || user.role == "Admin")) {
                res.json({ success: false, msg: "You don't have sufficient permissions" })
                return;
            }

            Cyclic.deleteOneById({ _id: user._id }, (err) => {
                if (err) {
                    res.json({ success: false, msg: "Error deleting user" });
                    return;
                }
                res.json({ success: true, msg: "Successfully deleted user" });
            })
        })
    });
})

One note - I've converted the condition destroyerUser.role == "Admin" || destroyerUser.role == "Moderator" to destroyerUser.role !== "Admin" && destroyerUser.role !== "Moderator" this is because I don't like a boolean NOT in front of long expressions:

if (!(destroyerUser.role == "Admin" || destroyerUser.role == "Moderator"))

It's easy to miss because you see the OR and the two expressions first. Luckily, that's very easily changeable using De Morgan's law for boolean expressions not (A or B) = not A and not B. Hence how I changed it to that.

You can further reduce some of the code duplication by adding a new function to handle all the errors. In all cases you do the same thing, aside from passing a different message, so you could just have:

const error = msg => res.json({ success: false, msg });

So, you can call error("User not found"), for example. It doesn't really reduce the amount of code you have, but it's more consistent this way. Also, if you decide to add more stuff to the error response (e.g., send additional headers, or you want to try and translate the error message), then you have a central location for it.

Answer 2

Well, at first, as @LawrenceCheron said, you should validate first, and not in the else statements.

instead of writing

if (destroyerUser) {
  // Do something
} else {
 // Exit
}

You can write

if (!destroyerUser) {
  // Exit
}

secondly, you can extract the userId to a variable, instead of chaining the objects keys, i.e

instead of writing

if (user.role == "Moderator" || user.role == "Admin") {
  res.json({ success: false, msg: "You don't have sufficient permissions" })
 }

you could write

const userRole = user.role;
if (userRole == "Moderator" || userRole == "Admin") {
  res.json({ success: false, msg: "You don't have sufficient permissions" })
}

and finally, complex statements such as

if (user._id.toString() != destroyerUser._id.toString()) {
 // ...
}

can be extracted to smaller functions

const isSameUserId = (id, sample) => id === sample;
if (!isSameUserId(user._id.toString(), destroyerUser._id.toString()) {
 // ...
}

or combining the above

const isSameUserId = (id, sample) => id === sample;

const userId = user._id.toString();
const destroyerId = destroyerUser._id.toString();

if (!isSameUserId(userId, destroyerId)) {
 // ...
}

Answer 3

One of my favorite styles or patterns for this sort of code is Early Termination (I don't know if it has a well known name but that's what I call it). Rather then nesting Happy Paths with Error Paths in the else. I put Error Paths in an Early Termination conditional.

if (!username) {
    res.json({ success: false, msg: "Username is not valid" });
    return;
}

try {
    // Get user a custom function you wrote to make sure it utilizes Promises
    let destroyerUser = await getUser(req.user.id);

    if (!destroyerUser) {
        res.json({ success: false, msg: "Destroyer user not found" });
        return;
    }

    if (destroyerUser.role != "Admin" && destroyerUser.role != "Moderator") {
        res.json({ success: false, msg: "You don't have sufficient permissions" })
        return;
    }

    // Continue this pattern all the way down
    // Nesting remains minimal, code is easy to follow
    // The trick is to invert your condition logic
    // then place a return so that all the code below won't execute
    // thus early termination when validation fails
}
catch(err) {
    res.json({ success: false });
    return;
}

And an example of how you can take a Callback Function and turn it into a Promise Function (which is necessary for Async/Await).

function getUser(userId) {
    // Return a promise to the caller that will be resolved or rejected
    // in the future. Callers can use Promise then or Await for a result.
    return new Promise((resolve, reject) => {
        User.getUserById(userId, (err, user) => {
            // If there is an error, call reject, otherwise resolve
            err ? reject(err) : resolve(user);
        });
    });
}