Server side validations and unit testing in MVP

Question

We are following passive controller approach and when user clicks on submit, server side validations are fired. There are many other fields on the screen that needs to be validated.

I would like to get feedback about the following approach.

1. Is it ok to have validations in Presenter.
2. As there are many fields in UI, PerformServerValidations() method is getting big. Is there any way I could refactor it.

3. Intention behind declaring PerformServerValidations() as public and returning a IList is to be able to test it. Is it a good approach to take?

   [TestMethod]
   public void Presenter_PerformServerValidations_ValidateFromCustomerId_ExpectFalseForInvalidNumber()
   {
    //creating presenter
    presenter.View.Stub(x=>x.FromCustomerId).Return("one two three"); 
    var errorCodes = presenter.PerformServerValidations();
    Assert.IsTrue(errorCodes.Contains("ERR_InvalidFromCustomerId"));
   }
   

Presenter.cs

public void OnSubmit()
{
 var serverValidationErrors = PerformServerValidations();
 this.View.ServerErrors = serverValidationErrors ; // View.ServerErrors will loop over the list and sets validator's IsValid property to false
 if(!serverValidationErrors.Any())
 {
 BindCustomers();
 }
}
public IList<string> PerformServerValidations()
{
 var errorCodes = new List<string>();
 int parsedFromCustomerId;
 int parsedToCustomerId;
 bool isValidFromCustomerId = int.TryParse(this.View.FromCustomerId, out 
parsedFromCustomerId);
 bool isValidToCustomerId = int.TryParse(this.View.ToCustomerId, out parsedToCustomerId);
 if(!string.IsNullOrWhiteSpace(this.View.FromCustomerId) && !isValidFromCustomerId)
 { 
 errorCodes.Add("ERR_InvalidFromCustomerId");
 }
 if(!string.IsNullOrWhiteSpace(this.View.ToCustomerId) && !isValidToCustomerId)
 { 
 errorCodes.Add("ERR_InvalidToCustomerId");
 }
 if((!string.IsNullOrWhiteSpace(this.View.FromCustomerId) && !string.IsNullOrWhiteSpace(this.View.FromCustomerId) && (isValidFromCustomerId && isValidFromCustomerId))
 { 
 if(parsedFromCustomerId > parsedFromCustomerId)
 {
 errorCodes.Add("ERR_InvalidCustomerIdRange");
 }
 }
 //There are many other fields to be validated
 return errorCodes;
}

• Malachi: please don't make the changes in the code, it invalidates the answers and makes it hard for someone new to the question follow along
• Malachi: add an Edit: to the question and post the correct code. when everything is fixed you could also post a new question as well...
• Malachi: you are welcome. if you have questions about how Code Review works just ask any of us in The Second Monitor and we will be more than happy to help you out. all the new questions are posted there by Stack Exchange as well.

Answer 1

I think you have a typo in the last outside if statement, I assume you meant two different variables when you wrote this:

if((!string.IsNullOrWhiteSpace(this.View.FromCustomerId) && !string.IsNullOrWhiteSpace(this.View.FromCustomerId) && (isValidFromCustomerId && isValidFromCustomerId))
{ 
 if(parsedFromCustomerId > parsedFromCustomerId)
 {
 errorCodes.Add("ERR_InvalidCustomerIdRange");
 }
}

parsedFromCustomerId will never be greater than parsedFromCustomerId

and the obvious one that won't cause a bug but probably won't give you the intended results is

 (isValidFromCustomerId && isValidFromCustomerId)

I assume you mean to put a different boolean variable in there instead of the same one twice.

this code won't function, correctly.

probably a Typo but you should proofread your code again just to make sure.

Answer 2

I'm not familiar with C#, just a few minor generic notes:

1. I guess you have a typo here, the following is always false:

   if(parsedFromCustomerId > parsedFromCustomerId) ...
   

2. Another one: the first condition is duplicated here (as well as the third one):

   if((!string.IsNullOrWhiteSpace(this.View.FromCustomerId) && !string.IsNullOrWhiteSpace(this.View.FromCustomerId) && (isValidFromCustomerId && isValidFromCustomerId))
   

3. I'd put the comment a line before:

   this.View.ServerErrors = serverValidationErrors ; // View.ServerErrors will loop over the list and sets validator's IsValid property to false
   

   The following is easier to read since it does not require any horizontal scrolling:

   // View.ServerErrors will loop over the list and sets validator's IsValid property to false
   this.View.ServerErrors = serverValidationErrors ; 
   

4. !string.IsNullOrWhiteSpace(this.View.FromCustomerId) is also used by the first if statement. I would create an explanatory variable for it:

   bool nonEmptyFromCustomerId = !string.IsNullOrWhiteSpace(this.View.FromCustomerId);
   if (nonEmptyFromCustomerId && !isValidFromCustomerId)
   { 
    errorCodes.Add("ERR_InvalidFromCustomerId");
   }
   ...
   if ((nonEmptyFromCustomerId) && ...
   ...
   

   (Clean Code by Robert C. Martin, G19: Use Explanatory Variables; Refactoring: Improving the Design of Existing Code by Martin Fowler, Introduce Explaining Variable)

5. If you are using the following statements multiple times consider creating a custom verification method for it:

   var errorCodes = presenter.PerformServerValidations();
   Assert.IsTrue(errorCodes.Contains("ERR_InvalidFromCustomerId"));
   

   For example:

   public void VerifyValidationErrorCodesContain(Presenter presenter, string expectedErrorCode)
   {
    var errorCodes = presenter.PerformServerValidations();
    Assert.IsTrue(errorCodes.Contains(expectedErrorCode));
   }