1
\$\begingroup\$

I am trying to spruce up my BDD skills using Specflow and Gherkin. Please see the classes below:

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

        public bool IsEligible(Customer customer)
        {
            if (customer.Expenditure < _expenditure)
            {
                return true;
            }
            else
            {
                return false;
            }
        }
    }

    public class Chestnut : IProduct
    {
        private const decimal _expenditure = 100;

        public bool IsEligible(Customer customer)
        {
            if (customer.Expenditure >= _expenditure)
            {
                return true;
            }
            else
            {
                return false;
            }
        }
    }

    public interface IProduct
    {
        bool IsEligible(Customer customer);
    }

    public interface IOfferCalculator
    {
        //Passed a Customer object instead of Expenditure and Gender variables because these variables will be validated
        //in the customer class.
        IEnumerable<IProduct> CalculateEligibility(Customer customer, IList<IProduct> products);
    }

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

        public Customer(Guid id, string gender, decimal expenditure)
        {
            //Validation logic to go here.
            Id = id;
            Gender = gender;
            Expenditure = expenditure;
        }

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


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

and the feature file below:

  • Feature: OfferCalculator

    • In order to calculate eligibility for offers
    • As an offer calculator I
    • want to be given a gender and expenditure
  • Scenario: Calculate eligibility when given a gender of male and an expenditure of 101

    • Given A customer with a gender of male and an axpenditure of 101
    • And All available products
    • When I calculate eligibility for products
    • Then Eligible for Concor

and the step definitions below:

[Binding]
    public class OfferCalculatorSteps
    {
        private Customer _customer;
        private IOfferCalculator _offerCalculator;
        private List<IProduct> _availableProducts;
        private List<IProduct> _actualProduct;

        [Given(@"A customer with a gender of (.*) and an axpenditure of (.*)")]
        public void GivenACustomerWithAGenderOfMaleAndAnAxpenditureOf(string gender, int expenditure)
        {
            _customer = new Customer(Guid.NewGuid(), gender, expenditure);
            _offerCalculator = new OfferCalculator();
        }

        [Given(@"All available products")]
        public void GivenAllAvailableProducts()
        {
            _availableProducts = new List<IProduct>();
            _availableProducts.Add(new Concor());
            _availableProducts.Add(new Chestnut());
        }

        [When(@"I calculate eligibility for products")]
        public void WhenICalculateEligibilityForProducts()
        {
            _actualProduct= _offerCalculator.CalculateEligibility(_customer, _availableProducts).ToList<IProduct>();
        }

        [Then(@"Eligible for Concor")]
        public void ThenEligibleForConcor()
        {
            //Could use a Set here instead of a list.
            var expectedCreditCard = new List<IProduct>();
            expectedCreditCard.Add(_availableProducts.OfType<Chestnut>().Single());
            Assert.Equal(expectedCreditCard, _actualProduct);
        }
    }

I would be grateful for comments with regards to the quality of this code (specifically the feature file and step definitions). I am specifically trying to follow the principle of least astonishment so that when other developers come on board; they can pick things up quickily. Here are some specific questions (though I am specifically asking for comments about the overall quality):

  1. Should features and step definitions be in their own namespace?

  2. Notice the scenario says: "And All available products". Would it be better to say: "And all the available products: Concor; Chestnut". I can't see a way of doing this as Concor and Chestnut are classes.

  3. Am I abusing the use of instance variables (I have four instance variables in the step definitions)?

  4. The feature says: "As an offer calculator". This is not a person. Is this normal?

  5. Notice I have done: expectedCreditCard.Add(_availableProducts.OfType<Chestnut>().Single());. Is this normal?

  6. Notice there are Guids referenced in the Step Definitions. Is this normal?

\$\endgroup\$
  • \$\begingroup\$ Please do not update the code in your question to incorporate feedback from answers, doing so goes against the Question + Answer style of Code Review. This is not a forum where you should keep the most updated version in your question. Please see what you may and may not do after receiving answers. \$\endgroup\$ – Mast Mar 29 '18 at 16:51
  • 1
    \$\begingroup\$ @t3chb0t You're right. Done. \$\endgroup\$ – Mast Mar 29 '18 at 16:57
  • \$\begingroup\$ @Mast, I only edited the name of a variable as it was confusing me and would confuse me (and maybe others) when I look back at this question in the future. However, I guess an edit is an edit. Thanks. \$\endgroup\$ – w0051977 Mar 31 '18 at 14:36
5
\$\begingroup\$

I am specifically trying to follow the principle of least astonishment

this and that

public IList<IProduct> _assignedProducts = new List<IProduct>();

don't get along well. A public field with an _ underscore? I'm more then surprised ;-)

But not only by that. If I can access this field directly, there is actually no need for me to want to use this method:

public void AddOffer(IProduct eligibleProduct)
{
    _assignedProducts.Add(eligibleProduct);
}

It would be a lot less surprising if the Customer class implemented the IEnumerable<IProduct> interface... it's also strange that you named the method AddOffer but the argument is IProduct.


public IEnumerable<IProduct> CalculateEligibility(Customer customer, IList<IProduct> products)
{
    foreach (var product in products)
    {
        if (product.IsEligible(customer))
        {
            yield return product;
        }
    }
}

You'll have to rethink this design. Here's another surprise. The customer has a collection of products so why do I have to specify both parameters separately to use CalculateEligibility? Shouldn't the AddOffer, or rather AddProduct already check this? The other thing is that this method does not caculate anything, it filters products by eligibility...

I'm still not happy with this but at least something like this would be more acceptable:

    public void AddProduct(IProduct product)
    {
        if(!product.IsEligible(this)) throw ...

        _products.Add(product);
    }

Since the product can by itself check the eligibility the OfferCalculator is completely unnecessary... but it wouldn't if it did its job propertly, this is, checking eligibility like the product currently does. You should move this logic to where it belongs and apparently it belongs to the OfferCalculator.

\$\endgroup\$
  • \$\begingroup\$ I have added an Update section to the question (I have not amended the question itself). I would be grateful for your comments on my update. I would also be grateful for any comments about the Specflow scenarios. \$\endgroup\$ – w0051977 Mar 27 '18 at 16:44
  • 1
    \$\begingroup\$ @w0051977 I'm not so sure your edit to the question is such a good idea. You should post them as comments under my answer... \$\endgroup\$ – t3chb0t Mar 27 '18 at 16:55
  • \$\begingroup\$ fair enough I will remove them. \$\endgroup\$ – w0051977 Mar 27 '18 at 16:56
  • \$\begingroup\$ I was actually asking for my Specflow tests to be reviewed. I am surprised that you have been so critical about the actual code as you seemed to approve of it when I had it reviewed here: codereview.stackexchange.com/questions/186148/…. Thanks for answering anyway. \$\endgroup\$ – w0051977 Mar 27 '18 at 16:58
  • \$\begingroup\$ @w0051977 but I didn't say it was good... I only said it was better than before ;-) I know you asked about your Specflow and I think this tool is extremely verbose. Such automatic tests work only in academic scenarios. There is no place for it in complex real world applications with several dependencies and complex test preparations. \$\endgroup\$ – t3chb0t Mar 27 '18 at 17:11
1
\$\begingroup\$

Sloppy left hand formatting in general

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

    public bool IsEligible(Customer customer)
    {
        if (customer.Expenditure < _expenditure)
        {
            return true;
        }
        else
        {
            return false;
        }
    }
}

Does not use private const string _gender

Can just return (customer.Expenditure < _expenditure);

Silly name and can be set to null.
public IList<IProduct> _assignedProducts = new List<IProduct>();

I am not getting OfferCalculator as a class. Why not a static method?

products.where(x => c.IsEligible(customer))
\$\endgroup\$
  • \$\begingroup\$ I have put the private variables in to help with readability. I have put OfferCalculator in there for testability and single responsibility. \$\endgroup\$ – w0051977 Mar 27 '18 at 16:52
  • \$\begingroup\$ How does a private variable not used help readability? \$\endgroup\$ – paparazzo Mar 27 '18 at 16:54
  • \$\begingroup\$ Thanks. I thought you were criticising the use of constants (I misread). I understand your point now. Does my reasoning about OfferCalculator make sense? \$\endgroup\$ – w0051977 Mar 27 '18 at 17:03
  • \$\begingroup\$ It does not make sense to me. But that does not mean it is wrong. \$\endgroup\$ – paparazzo Mar 27 '18 at 17:06
  • \$\begingroup\$ I think I am going to attempt this again without the OfferCalculator. Why would you have a static method? I am thinking about modifying Customer.AddProduct as per t3chbots suggestion. \$\endgroup\$ – w0051977 Mar 27 '18 at 19:28

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.