1
\$\begingroup\$

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>();
        _products.Add(_concor);
        _products.Add(_chestnut);
    }

    [Test]
    public void Should_receive_a_for_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);
        customer.AssignOffers(eligibleProducts.ToList());
        //Assert
        CollectionAssert.AreEqual(customer._assignedProducts, expectedConcor);
    }
}

Here is the supporting code:

public class Concor : IProduct
    {
        //Added constants
        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)
            {
                _assignedProducts.Add(product);
            }
        }
    }

    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?

\$\endgroup\$
  • \$\begingroup\$ This looks much better now! ;-) You could put the F & M constants in a GenderCode class and name them accordingly (female/male). \$\endgroup\$ – t3chb0t Jan 27 '18 at 19:21
  • \$\begingroup\$ 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. \$\endgroup\$ – t3chb0t Jan 27 '18 at 19:22
  • \$\begingroup\$ @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? \$\endgroup\$ – w0051977 Jan 27 '18 at 19:27
  • \$\begingroup\$ It can be either one (const/readonly) - the main point is, it should be as you've said immutable. \$\endgroup\$ – t3chb0t Jan 27 '18 at 19:29
  • \$\begingroup\$ With regards to the constant names. I will change _expenditure to: _minExpenditure. . I think I will leave Gender as it is? \$\endgroup\$ – w0051977 Jan 27 '18 at 19:33

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service, privacy policy and cookie policy

Browse other questions tagged or ask your own question.