trjhtr

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?

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.