0

Please see the code below:

public void GetOffers(List<Offer> offers)
        {
            if (Validate() == true)
            {
                OfferFactory offerFactory = new OfferFactory();
                foreach (Offer offer in offers)
                {
                    IValidator iValidator = offerFactory.getOffer(offer);
                    if (iValidator.Validate(this).IsValid==true)
                    {
                        _assignedOffers.Add(offer);
                    }
                }
            }
            else
            {
                throw new InvalidOperationException();
            }
        }

This method is contained in a class called: Applicant i.e. the applicant is validated against many (five offers). For example:

1) If the age is above 35 and less than 50 then they are entitled to offer 1
2) If the age is less than 35 and the gender is male then they are entitled to offer 2 
etc

Is it a bad idea to do this from an architectural perspective?

I am using Fluent Validation. The call to Validate; validates the application object i.e. it makes sure the first name contains a value and the surname contains a value etc.

The code then loops through all the offers to see if the member is entitled to them.

  • I am not sure what is going on in your code. The method accepts a list of offers, but it also gets a list of offers from a factory? Validate returns a Boolean in one place, but in another it returns an object with an IsValid property? And why does your system generate invalid offers to begin with? Maybe you can explain a bit more. – John Wu Jul 11 '17 at 19:56
  • @John Wu, thanks for that. I have edited the question. Does it make sense now? – w0051977 Jul 11 '17 at 20:11
  • This is a matching problem, not a validation problem. You are attempting to find out if the Applicant is a match for an offer. Validation is about determining if the Application put together correctly. I.e. finding missing information or incorrect sets of information. Using a validation library to do matching is going to confuse everyone maintaining the app down the road. – Berin Loritsch Jul 11 '17 at 20:16
  • Small stylistic note: you can drop the == true and the code will be easier to read and function exactly as it does now. – Berin Loritsch Jul 11 '17 at 20:17
  • @Berin Loritsch, is there a pattern or framework I can use for this? – w0051977 Jul 11 '17 at 20:19
2

Domain Driven Design is built on object oriented concepts, so we need to start there. We know we have an Applicant, and we have an Offer. The Applicant can be eligible for one Offer, but not a different one.

The question we have to ask ourselves is "who is responsible for determining eligibility?" Since the rules for eligibility are unique to each offer, I would make the Offer responsible for determining if an Applicant is eligible or not.

If we are in agreement here, then we have the following API that needs to exist for the Offer:

  • bool IsEligible(Applicant applicant)

The Applicant should not care about how the Offer determines his eligibility, only the result. So any supporting code needs to be encapsulated in the Offer class. It is not in the Applicant's domain.

Next, we need to determine how to apply the offers. You can make all kinds of arguments that are all correct as to whether the application of Offers to an Applicant should be done by the Applicant or a third-party we haven't discussed yet. For the sake of this answer, I'll use the solution you provided in your question: Applicant is responsible.

Now we need a method that takes all the available offers and attempts to apply them:

  • void AssignOffers(IEnumerable<Offer> availableOffers)

NOTE: I changed the name because you aren't getting anything. You are changing the internal state of your Applicant and applying the offers to them.

The content of that method should look something like this:

public void AssignOffers(IEnumerable<Offer> availableOffers)
{
    if (!IsValid)
    {
        throw new IllegalStateException("Can only apply offers to valid applicants");
    }

    _assignedOffers.AddRange(availableOffers
        .Where(offer => offer.IsEligible(this));
}

I have a property determining if this applicant is valid or not, but you can keep it a method. It might be appropriate if the logic is fairly complex.

On the other side we have our Offer class that we need some sort of implementation for our new method. If the Offer classes are created for each offer, then you can use hand written code. It would be easier to get started with for sure. However, let's say we need to generate the validation logic dynamically and you have a factory to do this. That factory should be named for what it creates. In this case it is a ValidationFactory that can generate the eligibility rules validator from your Offer. The implementation would look something like this:

public bool IsEligible(Applicant applicant)
{
    IValidator rules = _validationFactory.CreateForOffer(this);

    return rules.Validate(applicant).IsValid;
}

This keeps the domains and responsibilities clear. It also keeps implementation details from leaking into the rest of your code.

There's a few things to keep in mind with DDD:

  • Names matter. Classes and methods should be named for what they do. Classes are nouns and methods are verbs.
  • Intent should be clear. Don't use the verb Get when your method returns void.
  • Responsibilities should be clear.

The end result should be code that is relatively easy to understand and succinct.

  • Thanks. Are you suggesting that I move the validation of the applicant (for the offer) to the offer class. The ValidationFactory would then return subclasses of Offer? – w0051977 Jul 11 '17 at 21:35
  • I guess the subclasses would be named as per the offer e.g. ComputerOffer; TVOffer etc. Alternatively I could name them: Over30AndMaleOffer; Over30AndFemale offer etc? What do you think. +1. – w0051977 Jul 11 '17 at 21:36
  • Do you think it is "better" to store the business rules in the database (like in the other answer) or in the application. I prefer the database (as the rules are maintainable by a system admin), however, the DDD approach is telling me to do it in the code (domain model). – w0051977 Jul 11 '17 at 22:00
  • If you go this route, I wouldn't create a subclass for every offer-- that means you'd have to release code every time marketing comes up with a new offering (ugh). But you could potentially have subclasses for general categories of offers that have different behaviors, e.g. if some offers are for digital downloads and some are for shippable products. You'd still have to release a build when a new category comes up, but you'd also need other significant changes as well (e.g. hire some folks to work in fulfillment for shippable offers), so that kind of thing won't happen trivially anyway. – John Wu Jul 11 '17 at 22:19
  • For the discussion of DDD, a database is irrelevant. Your _validationFactory can generate the IValidation from a definition string. Whether that is stored in a database, a file, or whatever isn't particularly relevant to the domain design. – Berin Loritsch Jul 11 '17 at 22:35
1

"Validation" usually is applied to data that is beyond control, most commonly data entered by an end user, but in some situations you might validate back-end data as well if it is retrieved from a third party service. It would be unusual to "validate" data that you control.

Here is how I would refactor this:

  1. Applicant information such as name and date of birth would be validated via client side script in order to provide immediate feedback.

  2. The client-side validation above would be repeated server-side to catch any data manipulation that might be performed with Javascript turned off or via request capture/modification using a tool such as Paros. While it is "clean" to put that kind of validation in the business layer, the reality is that it is often convenient to do it in the presentation layer, since there are tools which allow you to mirror Javascript validation with server-side validation (e.g. Page.IsValid). But you could do it in either place.

  3. Once the applicant is fully validated, the site would ask the business layer to produce a list of offers valid for the user. It would be better to generate these offers endogenously... in other words, take the applicant's criteria as input and then produce the list. Iterating over the full list of all possible offers and validating them one by one would be less efficient, especially if the system contains many offers that the user is not eligible for.

To get the offer list, the most obvious way would be with a call that looks like this:

List<Offer> offers = offerFactory.GetOffers(applicant.DateOfBirth,
                                            applicant.OtherCriteria,
                                            applicant.Etc);

However, if there are a ton of fields, you might be tempted to pass the whole applicant instead, like this:

List<Offer> offers = offerFactory.GetOffers(applicant);

This raises the question of what input the GetOffers method expects. It could expect an Applicant, I suppose, like this:

class OfferFactory
{
    public List<Offer> GetOffers(Applicant applicant)
    {
        //Code goes here
    }
}

But that creates a dependency on the concrete Applicant class. It might be better to pass an interface. We can construe every applicant to also have application criteria, so we can define the Applicant class with an interface like this:

public interface IApplicationCriteria
{
    DateTime DateOfBirth;
}

class Applicant: IApplicationCriteria
{
    public DateTime DateOfBirth { get; set; }
    public string   FirstName   { get; set; } //Not in the interface
    public string   LastName    { get; set; } //Not in the interface
}

Then our GetOffers() implementation would accept the interface which contains only the data it needs:

public List<Offer> GetOffers(IApplicationCriteria criteria)
{
    //Code goes here
}

But you'd still call it the same way:

List<Offer> offers = offerFactory.GetOffers(applicant);

However, by passing the interface only, we create a better separation of concerns. This could be valuable if, for example, some day we decide that an Applicant doesn't have a FirstName and LastName but instead has a FullName. The GetOffers() call shouldn't care, and we can tell it doesn't care at compile time.

Meanwhile, GetOffers() would use the criteria to generate a query into the offers table

class OfferFactory
{
    private readonly SqlConnection _connection;

    public OfferFactory(SqlConnection connection)
    {
        _connection = connection; //Injected
    }

    public List<Offer> GetOffers(IApplicationCriteria criteria)
    {
        var offers = new List<Offer>();
        SqlCommand cmd = new SqlCommand("GetOffers");
        cmd.CommandType = CommandType.StoredProcedure;
        cmd.AddParameterWithvalue("@DateOfBirth", criteria.DateOfBirth);
        cmd.Connection = _connection;  
        SqlDataReader reader = cmd.ExecuteReader();
        while (reader.Read())
        {
            offers.Add(new Offer {
                                     OfferID   = reader["OfferID"]   as string,
                                     OfferName = reader["OfferName"] as string,
                                     Price     = reader["Price"]     as decimal
                                 }
                      );
        }

        return offers;
    }
}

This way you're not calling Validate over and over on data you know is already appropriate for the applicant.

If you really wanted to make as many layers as possible, you could move the data access to yet another layer, but in a case like this, where you're selecting a subset of data, it would be preferable to keep the business layer "dumb" and let the database find the records for you; it alone has the indexes and the data necessary to find the right offers without looking at all the offers that aren't valid.

How would the sproc look? Depends on the data structure, but it might go along the lines of this:

CREATE PROC GetOffers(@DateOfBirth DateTime)
AS
BEGIN
    SET NOCOUNT ON
    SET TRANSACTION ISOLATION LEVEL READ COMMITTED

    DECLARE @Age int
    DECLARE @OfferCount int

    --Compute the applicant's age
    SELECT @Age = DATEDIFF(hour, @dateOfBirth, GETDATE())/8766.0

    --Obtain the number of offers allowed
    SELECT @OfferCount = MAX(OfferCount)
    FROM   OfferAgeRules
    WHERE  MinAge <= @Age
    AND    MaxAge >= @Age

    --Get the offers
    SELECT TOP (@OfferCount)
             o.OfferID,
             o.OfferName,
             o.Price
    FROM     Offers o
    WHERE    o.MaxAge >= @Age
    AND      o.MinAge <= @Age
    AND      o.OfferStartDate < GETDATE()
    AND      o.OfferGoodUntil > GETDATE()
    ORDER BY o.Priority
END
  • Your GetOffer method contains data access logic? That rings alarm bells for me. – w0051977 Jul 11 '17 at 21:21
  • Well you can add another layer if you want. If you're comfortable not using an EF, and you're reasonably sure of your DB platform, it isn't completely necessary, but may be a good idea. OTOH I don't think it is a great idea to use an EF to get all possible offers and choose them in the business layer, if it is possible for the DB to select the offers using indexed fields and so on. Dragging everything over the wire is a waste of bandwidth. – John Wu Jul 11 '17 at 21:23
  • I like the rest of it though. +1. What would the stored procedure look like? I guess it means the business rules are stored in the database? – w0051977 Jul 11 '17 at 21:29
  • Well, rules that are likely to change during run time should be stored in the database anyway-- you can't really put them in code. I suppose the business layer could retrieve the rules separately and then apply them after the fact, but that would be wasteful. I provided a sample sproc that gets the rules from database tables and applies them. – John Wu Jul 11 '17 at 21:40
  • Thanks. However, what if the rules become more complex and perhaps cannot be stored on a database? – w0051977 Jul 11 '17 at 21:43

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.