2
\$\begingroup\$

Please see the code below:

public class Customer
{
    public int _iD { get; set; }
    public decimal _salary { get; set; }
    public List<String>_assignedOffers { get; set;  }
    private AbstractValidator<Customer> _validator;
    private IRulesOfferCalculator _rulesOfferCalculator;

    public Customer(AbstractValidator<Customer> validator, IRulesOfferCalculator rulesOfferCalculator)
    {
        _validator = validator;
        _rulesOfferCalculator = rulesOfferCalculator;
    }

    public void AssignOffers()
    {
        ValidationResult results = _validator.Validate(this);
        if (results.IsValid)
        {
            _assignedOffers = _rulesOfferCalculator.CalculateOffers(this);
        }
        else
        {
            _validator.ValidateAndThrow(this);
        }
    }
}

It is a Domain Layer class in a rich domain model. I have used FluentValidation for the validation. The Service Layer (WFC) creates an instance of Customer and initialises it.

The concern I have is that the class has two responsibilities:

  1. Validate the customer. If validation fails then thrown an exception. Validation can fail if the customer is under 21 years old etc

  2. If validation fails then throw an exception with reason for failure in the exception string. If validation is successful then assign offers (_assignedOffers is an instance variable)

I have two questions:

  1. Is it bad practice to throw an exception if validation fails? Is there a better way to notify the user if validation fails?

  2. Does this class have too many responsibilities i.e.

    1. validate and
    2. assign offers.

Alternatively the Service Layer could call: Validate and then call AssignOffers if Validate is successful. However, I believe this would make the domain model anemic.

Please be aware that I am building a rich domain model.

\$\endgroup\$
1
\$\begingroup\$

Is it bad practice to throw an exception if validation fails? Is there a better way to notify the user if validation fails?

It depends but usually validations return some kind of a validation result.


Does this class have too many responsibilities i.e. validate and assign offers.

Actually not. They are not the responsibilities of this class. Validation is injected and provided via a service so it's fine.

I would worry more about its design.

The constructor requries two parameters but their corresponding properties are not readonly so you could just use them. More appropriate would to use fields or make the properties get only and private.


public void AssignOffers()
{
    ValidationResult results = _validator.Validate(this);
    if (results.IsValid)
    {
        _assignedOffers = _rulesOfferCalculator.CalculateOffers(this);
    }
    else
    {
        _validator.ValidateAndThrow(this);
    }
}

It's a weird process. It first validates the object and then calculate the offer. The Offer property should be a getter-only property and use the calculator to get the value and the validation should take place where the other properties necessary for the calculation are set because they need to be validated as apparently they can be invalid.

\$\endgroup\$
  • \$\begingroup\$ Thanks. I have made some changes to the code in the OP. Could you take a look? Salary and DateOfBirth are still public because they are used by the validator i.e. a Person as to be over 21 years old etc. Does it make more sense now? \$\endgroup\$ – w0051977 Jun 26 '17 at 16:13
  • 1
    \$\begingroup\$ @w0051977 you are not allowed to modify the code. Please roll it back and refer to: What should I do when someone answers my question? \$\endgroup\$ – t3chb0t Jun 26 '17 at 16:13
  • \$\begingroup\$ @w0051977 otherwise someone else or I will roll it back anyway ;-] \$\endgroup\$ – t3chb0t Jun 26 '17 at 16:14
  • \$\begingroup\$ @ t3chb0t, thanks for the warning. I will roll it back now. \$\endgroup\$ – w0051977 Jun 26 '17 at 16:19
  • 1
    \$\begingroup\$ I have done that. I will reread your link about the rules of this site. Thanks. \$\endgroup\$ – w0051977 Jun 26 '17 at 16:22
0
\$\begingroup\$

The documentation of FluentValidation says, that the methods Validate and ValidateAndThrow are different ways to use the validator.

Therefore it is not required to use both. Something like the following should work just fine:

public void AssignOffers()
{
    _validator.ValidateAndThrow(this);
    _assignedOffers = _rulesOfferCalculator.CalculateOffers(this);
}

Does this class have too many responsibilities

Actually, I am a fan of small and stupid domain models. Why should the domain model have a dependency to the validator and the calculator? How do you handle e.g. validations that require more than one domain model? All that stuff can IMHO be done in the controller, view model, whatever....

But that is only my personal preference. If you decide using rich domain models, I think that is OK because the actual validation / calculation logic is abstracted as t3chbot already mentioned.


Is it bad practice to throw an exception if validation fails?

It depends. I would say, if validation failure is a common use case (e.g. validating user input), it would be better to use an validation approach without exceptions.

Otherwise, if validation failure is not expected (e.g. re-validating input from the client on the server), throwing an exception would be OK.

Is there a better way to notify the user if validation fails?

Hopefully you catch the validation exception and handle it because unhandled exceptions are not an appropriated way to notify the use about validation errors ;)

\$\endgroup\$
  • \$\begingroup\$ Thanks. How would I inform the user when a validation failure has occurred. Can you post some code? Thanks again. \$\endgroup\$ – w0051977 Jun 26 '17 at 18:35
  • \$\begingroup\$ @w0051977: That depends on the context where AssignOffers()is called. You could show a message box or a validation message somewhere on the GUI.... \$\endgroup\$ – JanDotNet Jun 26 '17 at 18:40
  • \$\begingroup\$ Thanks. The Person class is called from a service layer. The service layer is called by a few clients i.e. WPF, MVC4 and mobile. AssignOffers is called by the service layer. \$\endgroup\$ – w0051977 Jun 26 '17 at 18:51
  • \$\begingroup\$ @w0051977: That discussion is out of scope of the code review. For conceptual questions softwareengineering is the right place. \$\endgroup\$ – JanDotNet Jun 27 '17 at 5:32

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.