C# – Method to validate an object – should I have one method encapsulating all validation logic

csolidunit testing

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.

Best Answer

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.

Related Solutions

C# – Best way of validating Class properties

If I needed to input data and capture errors, I would probably do something more like:

public class Employee
{
    public int EmpID { get; set; }
    public string FirstName { get; set; }
    public string LastName { get; set; }
    public string Salary { get; set; }
}

public class ErrorEmployee : Employee
{
    public string[] ErrorReason { get; set; }
}

Read CSV file row by row
For each element i.e EmpId, FirstName... etc do the required validations by calling individual methods having validation logic. eg: public bool ValidateIsDecimal(string value) { } public bool ValidateIsEmpIdExists(string value) { } etc
If valid, fill corresponding property of "Employees" class. Add this Class to Employees collection (eg: List)
If NOT VALID, fill "ErrorReason" property, with appropriate reason as to what caused the validation to fail.(eg: Required filed was missing or datatype is not decimal etc) Add this Class to ErrorEmployees collection (eg: List)

I wouldn't want to pollute the regular Employee (singular, use the plural for a collection of them) object with error messages. As in the note above, check out the FluentValidation library for your validation and capture a list of errors rather than just the first found.

C# – Why aren’t all method virtual or why doesn’t have each class at least one interface

As Anders says, its partly about performance and partly about locking down poorly-thought designs down to reduce the scope of trouble caused later by people blindly inheriting things that were not designed to be inherited.

Performance seems obvious - while most methods wont be noticed on modern hardware, a virtual getter might cause quite a noticeable hit, even on modern hardware that does rely more and more on easily predicted instructions in the CPU pipeline.

Now the reason to make everything virtual by default seems to be driven by one thing only: unit testing frameworks. It seems to me that this is a poor reason, where we are changing code to suit the framework rather than designing it properly.

Perhaps the issue is with the frameworks used here, they should improve to allow replacement of functions at runtime with injected shims rather than building code that does this (with all the hacks to get round private methods as well as non-virtual ones, not to mention static and 3rd party functions)

So the reason methods are not virtual by default is as Anders said, and any desire to make them virtual to suit some unit test tool is putting the cart before the horse, proper design beats artificial constraints.

Now you can mitigate all of this by using interfaces, or by reworking your entire codebase to use components (or microservices) that communicate via message passing instead of directly wired method calls. If you enlarge the surface of a unit for testing to be a component, and that component is entirely self-contained, you don't really need to screw with it to unit test it.

Related Topic