Suppression de redondance dans les actions ASP.NET MVC

Question

Cette action est-elle trop redondante? Existe-t-il une meilleure façon de la simplifier?

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

Tous ces me semblent avoir une odeur de code ...

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

Answer 1

Ce changement semble conserver vos intentions d'origine un peu mieux:

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

Non, je dirais que vous avez seulement besoin du dernier IsValid vérification. Bien sûr, vous pouvez conserver l'avant-dernier, car changer le mot de passe s'il est le même que l'ancien peut entraîner des événements de journalisation inutiles ou autres, selon le cadre d'appartenance sous-jacent.

Answer 3

déplacer toutes vos validation de base comme la longueur de chaîne et le type au niveau du modèle, cela réduira beaucoup de code, vous pouvez consulter le cadre xval

Answer 4

Oui, je voudrais juste garder le dernier chèque IsValid, donc: Bien que, @ j-steen fasse un bon point au sujet de l'avant-dernier contrôle, en vous épargnant peut-être un peu de frais généraux.

Answer 5

déclarations emboîtées if peuvent aider à simplifier le 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(); 
}