2

I have a class, Customer which has some basic properties on it such as firstname, surname, email, dateofbirth. I have written another class called CustomerValidation which currently has one public method and three private methods. The public method is:

public bool ValidateCustomer(Customer customer)

This method calls three private methods which validate the name, email address and date of birth of the customer passed in, returning true only if all of these methods themselves return true.

However, even though it makes sense to me to validate a customer in one call, another part of me is thinking, if I unit test this, I could pass in a Customer with multiple invalid fields and not actually know if each private method is working correctly, as just one of these methods returning false would give me a potentially false positive.

It doesn't make sense to me to write tests to the public API that target the implementation detail. So I'm wondering whether I should make all the methods public, which still doesn't feel right or refactor the methods out into their own classes and rework my validation logic.

  • Do you want to know what exactly went wrong? – Andy Apr 19 '16 at 12:30
  • 1
  • 2
    As a rule I would not allow a customer object to be in an invalid state. Missing (or empty) email might be ok an illegal email not. If you allow your customer objects to be mutable, the validation should happen in the properties. That validation can of course be done in another class reusing the validation of emails in other classes. – Bent Apr 19 '16 at 12:43
  • 2
    How hard would it be to create Customer objects which have exactly one of the fields invalid and the others valid, so you test all three "implementation details" with only the one public method? (assuming it makes any sense to allow "invalid Customers" to be constructed in the first place) – Ixrec Apr 19 '16 at 12:50
  • 1
    An alternative perspective to take would be to say: if I pass in an instance with what I believe to be a single incorrect field and it fails the overall validation haven't I proven that the validation method works? It's none of my business which sub-validation method it's calling internally that returns false, as long as the Validation unit behaves as expected. – combinatorics Apr 19 '16 at 14:15
3

Instead of bool you could return some integer error code or even validation message in string. This would provide you a knowledge of what failed the validation. The string solution have also particular advantage because you already have composed message to inform end user about results of validation.

  • I had actually written down an idea to refactor my validation class to return more useful information which would also make it far more testable. I like this solution and think it's better than any of those that I proposed above. – BenM Apr 19 '16 at 14:39
2

In Ruby on Rails, which places validation logic in the model rather than in the controller or otherwise closer to the front end, the pattern used is to provide both a Boolean and an error dictionary. After an object has been validated, you can access the errors dictionary which has property names as keys, and a set of error description strings as the value. Then custom validation methods must add an error string for the right property key, and the is_valid method returns true if the error dictionary is empty. (Note: links are to rails documentation to give you more details of the pattern used there).

This approach gives the simplicity of a single Boolean validation method but the verbosity of per property error messages.

The way that it's commonly hooked up to the view is this:

// in the controller:
// update someModel based on query parameters
if(someModel.IsValid()) {
  // go to the success page
} else {
  // render the same edit form, but use the errors dictionary to display the error string above each invalid input 
  renderEditPage(someModel.Errors);
}

Edit: Comparing with @Kaa's answer

Mine is mostly the same, with the following exceptions:

  1. error dictionary instead of error string
  2. error dictionary as separate property rather than a return value
  3. some links to the RoR documentation.

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service, privacy policy and cookie policy

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