5
\$\begingroup\$

I asked a similar question yesterday, which was criticised because I posted the test without the supporting code. Therefore I deleted the question yesterday as it was not clear enough. I am now going to post it again with the supporting code. Please see the test code below:

[TestFixture]
public class CustomerTest
{
    private IList<Product> _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<Product>();
        _products.Add(_concor);
        _products.Add(_chestnut);
    }

    [Test]
    public void Should_receive_a_for_concor()
    {
        var expectedConcor = new List<Product>();
        expectedConcor.Add(_concor);
        var gender = "F";
        var expenditure = 1500M;
        var Customer = new Customer(gender,expenditure);
        //Act
        Customer.AssignOffers(_offerCalculator, _products);
        //Assert
        CollectionAssert.AreEqual(Customer._assignedProducts, expectedConcor);
    }
}

Here is the supporting code:

public class Concor : Product
    {
        public override bool Check(Customer customer)
        {
            if (customer._expenditure > 100 && customer._gender == "F")
            {
                return true;
            }
            else
            {
                return false;
            }
        }
    }

    public class Chestnut : Product
    {
        public override bool Check(Customer customer)
        {
            if (customer._expenditure >  100 && customer._gender =="M")
            {
                return true;
            }
            else
            {
                return false;
            }
        }
    }

    public abstract class Product
    {
        public abstract bool Check(Customer customer);
    }

    public interface IOfferCalculator
    {
        IEnumerable<Product> Calculate(Customer customer, IList<Product> products);
    }

    public class Customer
    {
        public string _gender { get; protected set; }
        public decimal _expenditure { get; protected set; }
        public IList<Product> _assignedProducts = new List<Product>();

        public Customer(string gender, decimal expenditure)
        {
            _gender = gender;
            _expenditure = expenditure;
        }

        public void AssignOffers(IOfferCalculator offerCalculator, IList<Product> products)
        {
            foreach (var product in offerCalculator.Calculate(this, products))
            {
                if (!_assignedProducts.Contains(product))
                {
                    _assignedProducts.Add(product);
                }
            }
        }
    }

    public class OfferCalculator : IOfferCalculator
    {
        public IEnumerable<Product> Calculate(Customer customer, IList<Product> products)
        {
            foreach (var product in products)
            {
                if (product.Check(customer))
                {
                    yield return product;
                }
            }
        }
    }

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\$
7
\$\begingroup\$

You are asking about your tests but there are issues in your code that should be addressed before you start testing.


public abstract class Product
{
    public abstract bool Check(Customer customer);
}

You made Product an abstract class but this is not necessary. It does not provide any default implementation for anything so an simple interface would be enough.

There is however one more issue with it. The Check method. It doesn't say anything about what is being checked.

customer._expenditure >  100 && customer._gender =="M"

Its implementation evaluates the above condition but there is no information about it in the method name so nobody could tell what it checks.

On top of it it even uses the hardcoded magic-number 100. This should be a constant.

You also put it inside an if/else which isn't necessary. You can just return the result.


public void AssignOffers(IOfferCalculator offerCalculator, IList<Product> products)
{
    foreach (var product in offerCalculator.Calculate(this, products))
    {
        if (!_assignedProducts.Contains(product))
        {
            _assignedProducts.Add(product);
        }
    }
}

This is the method you test but it's difficult to test it because it's doing to much: it filters the product list, it checks whether it's assigned and it adds it to another list. To me these are three different responsibilities. You should at least pass an already filtered product list.

The method also does not inform me that anything will be filtered at all. It says AssignOffers but instead of assing offers it calculates something and it works with producs that it adds to another list but it does not assign anything. Everything is misleading about this method and it's full of surprises.


Having said that I find all your tests are meaningless because the implementation is meaningless too. The code is confusing and doing completely different things that the names would suggest. Writing tests for such code is pointless.

You should first fix the main code so that it's clear what it does so that you know what you test. Try to pick names that clearly communicate the purpose and do exacly only that.

For exmaple if you Calculate something then let this method return a result of this calculation based on as few parameters as possible. Passing entire objects where you only need two properties is a code smell.


public IEnumerable<Product> Calculate(Customer customer, IList<Product> products)
{
    foreach (var product in products)
    {
        if (product.Check(customer))
        {
            yield return product;
        }
    }
}

But wait a minute! Let's take a look at what Calculate actually does... to my (not) surprise it does not calculate anything but instead it checks products against a customer!!! How confusing is that?


This is such a mess that you can throw all these tests away and first rewrite the code completely then write new tests.

\$\endgroup\$
  • \$\begingroup\$ Many thanks +1. I will make the changes you suggest and post another question. \$\endgroup\$ – w0051977 Jan 27 '18 at 13:32
  • \$\begingroup\$ You say that AssignOffers is doing too much and doesn't assign anything. It assigns an offer (product) to a list. What is wrong with that? Also how does it do too much? Thanks. \$\endgroup\$ – w0051977 Jan 27 '18 at 13:59
  • \$\begingroup\$ @w0051977 1) yes it does do too much: it filters the list with Calculate (!!!), it checks whether the list Contains a product and only then it adds a product to a list so it does three different things. 2) adding with Add is not the same as assignment with the = sign which would mean product.Offer = offer or something similar. But this is not what this method is doing. \$\endgroup\$ – t3chb0t Jan 27 '18 at 14:04
  • \$\begingroup\$ I see. So if I remove the if statement then it is simple enough? Thanks. The reason I have the check is because I used a hashset instead of a list. I guess it is ok to have the if statement if there is a hashset? \$\endgroup\$ – w0051977 Jan 27 '18 at 14:06
  • \$\begingroup\$ @w0051977 sorry, I cannot answer this question because I don't know what you are going to achieve. Ask yourself what is this (or any other) method supposed to do and do only that. No filtering no anything else. If it needs to filter something then create another method that only filters producs and give the first one a filtered list. \$\endgroup\$ – t3chb0t Jan 27 '18 at 14:13

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.