trjhtr

Following on from this question. I have made a very small change (changes are commented on in the code). Please see the test code below:

[TestFixture]
public class CustomerTest

 private IList<IProduct> _products;
 private Concor _concor;
 private Chestnut _chestnut;

 private IOfferCalculator _offerCalculator;

 [OneTimeSetUp]
 public void OneTimeSetUp()
 
 _offerCalculator = new OfferCalculator();
 

 [SetUp]
 public void Setup()
 
 _concor = new Concor();
 _chestnut = new Chestnut();
 _products = new List<IProduct>();
 _products.Add(_concor);
 _products.Add(_chestnut);
 

 [Test]
 public void Should_receive_a_concor()
 
 var expectedConcor = new List<IProduct>();
 expectedConcor.Add(_concor);
 var gender = "F";
 var expenditure = 1500M;
 var customer = new Customer(gender,expenditure);
 //Act
 var eligibleProducts = customer.GetEligibleOffers(_offerCalculator, _products);
 //refactored this code to loop through the eligible products
 foreach (IProduct eligibleProduct in eligibleProducts)
 
 customer.AddOffer(eligibleProduct);
 
 CollectionAssert.AreEqual(customer._assignedProducts, expectedConcor);
 

and the supporting code below:

public class Concor : IProduct
 
 private const decimal _expenditure =100;
 private const string _gender = "F";

 public bool IsEligible(string gender, decimal expenditure)
 
 if (expenditure > _expenditure && gender == _gender)
 
 return true;
 
 else
 
 return false;
 
 
 

 public class Chestnut : IProduct
 
 private const decimal _expenditure = 100;
 private const string _gender = "M";

 public bool IsEligible(string gender, decimal expenditure)
 
 if (expenditure > _expenditure && gender == _gender)
 
 return true;
 
 else
 
 return false;
 
 
 

 public interface IProduct
 
 bool IsEligible(string gender, decimal expenditure);
 

 public interface IOfferCalculator
 
 IEnumerable<IProduct> CalculateEligibility(string gender, decimal expenditure, IList<IProduct> products);
 

 public class Customer
 
 public string Gender get; protected set; 
 public decimal Expenditure get; protected set; 
 public IList<IProduct> _assignedProducts = new List<IProduct>();

 public Customer(string gender, decimal expenditure)
 
 Gender = gender;
 Expenditure = expenditure;
 

 public IEnumerable<IProduct> GetEligibleOffers(IOfferCalculator offerCalculator, IList<IProduct> products)
 
 return offerCalculator.CalculateEligibility(Gender, Expenditure, products);
 

 //Refactored this method to only accept one product
 public void AddOffer(IProduct eligibleProduct)
 
 _assignedProducts.Add(eligibleProduct);
 
 

 public class OfferCalculator : IOfferCalculator
 
 public IEnumerable<IProduct> CalculateEligibility(string gender, decimal expenditure, IList<IProduct> products)
 
 foreach (var product in products)
 
 if (product.IsEligible(gender,expenditure))
 
 yield return product;
 
 
 
 

I have simply made the following changes:

1. Renamed AssignOffer to AddOffer
   


2. 
   AddOffer accepts a single product rather than a list of products


3. The test method loops through the products instead of the Customer class

The reason I have made this change is to be more consistent with what I am seeing online like here and here. Notice that single entities are added to the collections in both links.

I cannot find any examples similar to the way I approached it in my other question.

I would be grateful for comments on, which approach to use as I am trying to follow the principle of least astonishment.

Update

A change is needed to Chestnut.IsEligible(). The first line of the if statement needs to change to: if (expenditure < _expenditure && gender == _gender). Notice the greater than operator changed to a less than operator.

Please note that Concor is not affected.

I do not see any big issue, just few minor design details.

Both Coconut and Chestnut implements IProduct and their implementation is the same. This makes me consider two alternatives:

• In this case inheritance maybe is not the best tool for this job. I does not know for sure because I cannot see the big picture but someone strongly disagree to design a hierarchy just to add a state, without any added/changed behavior. This is not a suggestion but more a thing to consider (also according to your database design).


• Introduce an abstract base class Product where you put the shared implementation (Do not Repeat Yourself).

Also note that IsEligible() may be slightly simplified:

abstract class Product : IProduct

 public bool IsEligible(string gender, decimal expenditure)
 => expenditure > _expenditure && gender == _gender;

Now you have to choose, inheritance or not? If not then you can go one step further: make Gender and Expenditure two public properties, add a Name property make Product a concrete class and...read the product list from database. This is definitely the most flexible structure because you do not have to introduce more and more code when adding new products. I imagine this pseudo-code:

var customer = new Customer(...);
var offerCalculator = new BlackFridayOfferCalculator(...);

var eligibleProducts = customer.GetEligibleOffers(offerCalculator, database.Products);

In the other case (keeping hierarchy in place) you just need to add a protected ctor to Product to accept gender and expenditure parameters (which are also private readonly fields in the base class).

An important note: with this code approach you need to have all your entities in memory (it depends on the ORM you're using but I suppose they can't track down virtual method invocations). It means that your queries will consume a lot of memory and CPU and your super optimized fine-tuned database engine with indices won't be able to help. Think twice about this.

<EDIT>

Few quick thoughts about your edited question:

How many different products you have? If they're just FEW (and you're reasonably sure that they'll remain few in near future) then I'd keep the hierarchy (but note the paragraph about performance, probably ALL your entities will be retrieved and materialized in memory outside the DBE). If, however, you have a fair amount of products, they'll grow in future or they must be managed somehow (for example to add/remove products or to change their properties) then hard-coded solution is absolutely a no-no and you definitely should have a generic Product class (not abstract).

On the other side to have different IsEligible() expressions will quickly make your code much harder to write. You can start easily with a simple flag (expenditure greater or lower than) but for more complex scenarios I'd introduce some external business logic. If calculation is done in memory then it's easy: pick an expression evaluator like NCalc and move the condition to database (as simple text).

Note that code generation here may help you: it's fairly easy to read a configuration/list of products and to generate all the code you need (with hierarchy or not) and also producing all the relevant SQL code. Sometimes I used NCalc expressions in conjunction with its ExpressionVisitor to generate SQL, JavaScript, and plain English translations of my business logic (expressed as an NCalc expression and evaluated as such in C#). Without much more knowledge about your scenario and domain I can't suggest anything better but just give you few (there are many more options...) ideas to begin playing with.

</EDIT>

Now few minor details. You have a field Gender. Let's refine this concept step by step. Now you're using == (current culture, case sensitive) for comparison. What you probably want is to use invariant culture comparison (eventually case insensitive) or even just ordinal comparison. Use String.Equals() and make it explicit.

However gender does not need to be a string. It might be a simple enum with two values Male and Female.

However gender defines the sexual orientation of a person. It's not the sex at birth. If you stick with the gender then you should definitely provide much more options (Agender, Bigender, Demigender, Intergender, Non-binary, Cisgender, Transgender and so on, just to mention few). If you need to know customer's gender then you should pick a good list, read it carefully and stick to it. Do not underestimate this aspect because gender is an important and delicate topic.

On the same line you should note that these definitions are fluid and subject to change, if you're not using sex at birth (or sex assigned at birth) then you better move this list into a separate database table. Also note that what it's acceptable to present in this list may vary according to the country then logic behind this is much much more complex than you may expect. There are also privacy implications you can't ignore...are you still sure you need to know customer's gender instead of assigned sex at birth?