Take the 2-minute tour ×
Code Review Stack Exchange is a question and answer site for peer programmer code reviews. It's 100% free, no registration required.

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;
}
share|improve this question
1  
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 Apr 5 at 16:25
    
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 Apr 5 at 16:26
1  
Thank you. I got it. –  Sunny Apr 5 at 16:28
    
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. –  Malachi Apr 5 at 16:32

2 Answers 2

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.

share|improve this answer

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));
    }
    
share|improve this answer
1  
Thank you. 3,4 & 5 are helpful :-) –  Sunny Apr 5 at 16:26

Your Answer

 
discard

By posting your answer, you agree to the privacy policy and terms of service.

Not the answer you're looking for? Browse other questions tagged or ask your own question.