Redunancy elimination in ASP.NET MVC Actions

Question

Is this action too redundant - is there a better way to simplify it?

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

All of these seem to me to have a code smell...

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

Answer 1

No, I'd say you only need the last IsValid-check.

Of course, you might want to keep the second to last one, as changing the password if it's the same as the old one might result in unnecessary log-events or whatnot, depending on the underlying membership framework.

Answer 2

move all your basic validation like string length and type to the model level, this will reduce lots of code, you can check out the xval framework

Answer 3

Yeah, I would just keep the last IsValid check, so:

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

Although, @j-steen makes a good point about the second to last check possibly saving you some overhead.

Answer 4

Nested if statements may help simplify the code:

[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 5

This change seems to preserve your original intentions a little better:

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