Устранение избыточности в действиях ASP.NET MVC

Question

Является ли это действие слишком избыточным - есть ли лучший способ упростить его?

[Authorize, AcceptVerbs(HttpVerbs.Post)]
public ActionResult ChangePassword(string oldPassword, string newPassword, string confirmPassword)
{
    var oldPasswordValidationResults = _validatorProvider.Validate<IStringLengthValidator>(oldPassword);
    oldPasswordValidationResults.Where(r => !r.Passed)
                                .Each(r => ModelState.AddModelError("OldPassword", "Please enter your old password."));


    var newPasswordValidationResults = _validatorProvider.Validate<IStringLengthValidator>(newPassword);
    newPasswordValidationResults.Where(r => !r.Passed)
                                .Each(r => ModelState.AddModelError("NewPassword", "Please enter a new password."));

    if (!ModelState.IsValid)
        return View();

    if (newPassword != confirmPassword)
        ModelState.AddModelError("ConfirmPassword", "The passwords do not match.");

    if (!ModelState.IsValid)
        return View();

    if (!_userMembershipService.ChangePassword(oldPassword, newPassword))
        ModelState.AddModelError("_FORM", "Unable to change your password.");

    if (!ModelState.IsValid)
        return View();

    return View("ChangePasswordSuccessful");
}

Мне кажется, что все это имеет запах кода ...

if (!ModelState.IsValid)
    return View();

Answer 1

Это изменение, кажется, немного лучше сохраняет ваши первоначальные намерения:

if (newPassword != confirmPassword)
{
    ModelState.AddModelError("ConfirmPassword", "The passwords do not match.");
    return View();
}

if (!_userMembershipService.ChangePassword(oldPassword, newPassword))
{
    ModelState.AddModelError("_FORM", "Unable to change your password.");
    return View();
}

return View("ChangePasswordSuccessful");

Answer 2

Вложенные if операторы могут помочь упростить код:

[Authorize, AcceptVerbs(HttpVerbs.Post)]
public ActionResult ChangePassword(string oldPassword, string newPassword, string confirmPassword)
{
    var oldPasswordValidationResults = _validatorProvider.Validate<IStringLengthValidator>(oldPassword);
    oldPasswordValidationResults.Where(r => !r.Passed)
                                .Each(r => ModelState.AddModelError("OldPassword", "Please enter your old password."));


    var newPasswordValidationResults = _validatorProvider.Validate<IStringLengthValidator>(newPassword);
    newPasswordValidationResults.Where(r => !r.Passed)
                                .Each(r => ModelState.AddModelError("NewPassword", "Please enter a new password."));

    if (ModelState.IsValid) {
        if (newPassword == confirmPassword) {
            if (_userMembershipService.ChangePassword(oldPassword, newPassword)) {
                return View("ChangePasswordSuccessful");
            }
            else {
                ModelState.AddModelError("_FORM", "Unable to change your password.");
            }
        }
        else {
            ModelState.AddModelError("ConfirmPassword", "The passwords do not match.");
        }
    }

    return View();
}

Answer 3

переместите все ваши основные проверки, такие как длина строки и тип, на уровень модели, это уменьшит объем кода, вы можете проверить xval framework

Answer 4

Да, я бы просто сохранил последнюю проверку IsValid, поэтому:

[Authorize, AcceptVerbs(HttpVerbs.Post)]
public ActionResult ChangePassword(string oldPassword, string newPassword, string confirmPassword)
{
    var oldPasswordValidationResults = _validatorProvider.Validate<IStringLengthValidator>(oldPassword);
    oldPasswordValidationResults.Where(r => !r.Passed).Each(r => ModelState.AddModelError("OldPassword", "Please enter your old password."));


    var newPasswordValidationResults = _validatorProvider.Validate<IStringLengthValidator>(newPassword);
    newPasswordValidationResults.Where(r => !r.Passed).Each(r => ModelState.AddModelError("NewPassword", "Please enter a new password."));


    if (newPassword != confirmPassword)
        ModelState.AddModelError("ConfirmPassword", "The passwords do not match.");

    if (!_userMembershipService.ChangePassword(oldPassword, newPassword))
        ModelState.AddModelError("_FORM", "Unable to change your password.");

    if (!ModelState.IsValid)
        return View();

    return View("ChangePasswordSuccessful");
}

Несмотря на то, что @ j-steen замечательно подходит ко второй и последней проверке, возможно, сэкономит вам немного времени.

Answer 5

Нет, я бы сказал, что вам нужна только последняя проверка IsValid.

Конечно, вы можете оставить второй до последнего, так как изменение пароля, если оно совпадает со старым, может привести к ненужным событиям журнала или еще чем-то, в зависимости от базовой структуры членства.