# Testing the process of assigning offers to a customer 2

I asked this question earlier: Testing the process of assigning offers to a customer. Following on from the answerers comprehensive response. I have attempted it again. 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>();
}

[Test]
{
var expectedConcor = new List<IProduct>();
var gender = "F";
var expenditure = 1500M;
var customer = new Customer(gender,expenditure);
//Act
var eligibleProducts = customer.GetEligibleOffers(_offerCalculator, _products);
customer.AssignOffers(eligibleProducts.ToList());
//Assert
CollectionAssert.AreEqual(customer._assignedProducts, expectedConcor);
}
}


Here is the supporting code:

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

public bool IsEligible(string gender, decimal expenditure)
{
//Added constants.  Not sure whether they should be private or public.  Surely they should be private and not public just so users
//of the Domsain Model can see them?
if (expenditure > _expenditure && gender == _gender)
{
return true;
}
else
{
return false;
}
}
}

public class Chestnut : IProduct
{
//Added constants.  Not sure whether they should be private or public.  Surely they should be private and not public just so users
//of the Domsain Model can see them?
private const decimal _expenditure = 100;
private const string _gender = "M";

public bool IsEligible(string gender, decimal expenditure)
{
//Remove magic string and magic decimal
if (expenditure > _expenditure && gender == _gender)
{
return true;
}
else
{
return false;
}
}
}

//Replaced abstract class with interface
public interface IProduct
{
//This method now accepts parameters rather than an object
bool IsEligible(string gender, decimal expenditure);
}

public interface IOfferCalculator
{
//Renamed this method.
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;
}

//Added this method as a result of refactoring AssignOffers.
public IEnumerable<IProduct> GetEligibleOffers(IOfferCalculator offerCalculator, IList<IProduct> products)
{
//Pass Gender and Expenditure instead of this object (Customer).
return offerCalculator.CalculateEligibility(Gender, Expenditure, products);
}

//Refactored this method.
public void AssignOffers(IList<IProduct> products)
{
foreach (var product in products)
{
}
}
}

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 included comments to show what I have changed.

I would like some feedback about the quality of the test. The company like to use BDD, however I am conscious that I asserting state rather than behaviour (arguing that I am testing calculations rather than orchestrations). I would also like some feedback about my use of the: OneTimeSetup attribute and Setup attribute. Also How would I test for a Chestnut? Add a new method or use an attribute for the test cases?

• This looks much better now! ;-) You could put the F & M constants in a GenderCode class and name them accordingly (female/male). – t3chb0t Jan 27 '18 at 19:21
• Well, 100 is a constant now but the name, ouch! The name should explain why it's 100. Is it max or min or some other expenditure? Name it properly. – t3chb0t Jan 27 '18 at 19:22
• @t3chb0t, if I encapsulate the gender in a immutable GenderCode class then I will have to change the IProduct._gender constant to a read only property. I assume that is what you mean? – w0051977 Jan 27 '18 at 19:27
• It can be either one (const/readonly) - the main point is, it should be as you've said immutable. – t3chb0t Jan 27 '18 at 19:29
• With regards to the constant names. I will change _expenditure to: _minExpenditure. . I think I will leave Gender as it is? – w0051977 Jan 27 '18 at 19:33