Избегайте обратных вызовов и вложенных операторов if - PullRequest
0 голосов
/ 29 марта 2020

Как я могу избежать этого беспорядка ??, я хотел бы как-то сжать код, о обратных вызовах, которые мне больше всего понравятся, используя функции ожидания / обещания javascript, но для того, чтобы вложить, если я действительно не ' не знаю что делать Любые советы по рефакторингу этого беспорядка были бы отличными. Спасибо.

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" });
    }
})

Ответы [ 3 ]

1 голос
/ 29 марта 2020

Ну, во-первых, как сказал @LawrenceCheron, вы должны сначала проверить, а не в выражениях else.

вместо записи

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

Вы можете написать

if (!destroyerUser) {
  // Exit
}

во-вторых, вы можете извлечь userId в переменную вместо цепочки ключей объектов, то есть

вместо записи

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

вы можете написать

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

и, наконец, сложные операторы, такие как

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

, могут быть извлечены в меньшие функции

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

или объединены выше

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

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

if (!isSameUserId(userId, destroyerId)) {
 // ...
}
1 голос
/ 29 марта 2020

Есть несколько вещей, которые вы можете сделать с помощью вложенных боковых пирамид. Один из довольно универсальных методов - сначала обработать ошибки , а затем выполнить обычный код. Итак, допустим, у вас есть:

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

Это OK , но вы действительно столкнетесь с проблемами, как только получите

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

Вы сразу начнете складывать больше и больше блоков. Более того, логика c для успеха, как правило, очень большая, так как вам нужно будет сделать больше вещей позже. И logi c обработки ошибок может быть одной строкой. Так что, если вы хотите узнать, что происходит, при ошибке вам нужно много прокрутить.

Чтобы избежать этого, вы можете выйти рано. Шаги для преобразования вашего кода одинаковы:

  1. Переверните условие if, которое проверяет на успешность, вместо этого проверьте на ошибки
  2. Поменяйте местами блок if и else block.
  3. Добавьте return к if.
  4. Снимите блок else и сгладьте его.

И вы получите следующее

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

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

//do next step

Теперь вы удаляете вложенность и отбрасываете блоки else, поскольку вы просто набираете return и выходите из функции. Если ваша обработка ошибок - это оператор throw, который уже существует, значит, вам не нужно return.

Следующее, что у вас есть несколько дублирующих логи c здесь:

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" });
            }
        })
    }
}

Независимо от того, является ли destroyerUser администратором или модератором, вы в любом случае называете одинаковым deleteByOne. И вы также возвращаете одно и то же сообщение об успехе в обоих случаях. Итак, у вас есть шесть различных веток в коде, тогда как у вас есть только три результата:


+----------------+-----------+-----------------+-----------------------------------------+
| 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"                   |
+----------------+-----------+-----------------+-----------------------------------------+

Вместо этого вы можете просто проверить, выполняет ли модератор удаление сначала, и если они делают имеют достаточные права, они администратор, затем просто выполните удаление один раз:

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" });
})

Итак, если мы применим эти две вещи:

  • инвертируем логи c и досрочно выйдите
  • удалите дублирующий код

Тогда ваш код будет выглядеть так:

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" });
            })
        })
    });
})

Одна заметка - я преобразовал условие destroyerUser.role == "Admin" || destroyerUser.role == "Moderator" to destroyerUser.role !== "Admin" && destroyerUser.role !== "Moderator" это потому, что мне не нравится логическое НЕ перед длинными выражениями:

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

Легко пропустить, потому что сначала вы видите OR и два выражения. К счастью, это очень легко изменить, используя закон Де Моргана для логических выражений not (A or B) = not A and not B. Следовательно, как я изменил это.

Вы можете еще больше уменьшить дублирование кода, добавив новую функцию для обработки всех ошибок. Во всех случаях вы делаете то же самое, кроме передачи другого сообщения, поэтому вы можете просто получить:

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

Итак, вы можете позвонить, например, error("User not found"). Это на самом деле не уменьшает объем кода, который у вас есть, но он более последовательный. Кроме того, если вы решите добавить дополнительные сведения в ответ об ошибке (например, отправить дополнительные заголовки или попытаетесь перевести сообщение об ошибке), у вас есть центральное расположение для него.

1 голос
/ 29 марта 2020

Один из моих любимых стилей или шаблонов для такого рода кода - «Раннее завершение» (я не знаю, имеет ли оно хорошо известное имя, но я так его называю). Скорее, вложив Happy Paths с путями ошибок в else. Я поместил Пути ошибок в условное обозначение «Раннее завершение».

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;
}

И пример того, как вы можете взять функцию обратного вызова и превратить ее в функцию обещания (которая необходима для 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);
        });
    });
}
Добро пожаловать на сайт PullRequest, где вы можете задавать вопросы и получать ответы от других членов сообщества.
...