5
\$\begingroup\$

I am implementing strategy pattern. I have defined a strategy as interfaces and concrete classes to implement the strategy. Based on user selection/configuration, the algorithm to apply the discount changes.

  public interface IStrategy
    {
        double calculate(double x, double y);
    }

concrete classes implementing the gold strategy is listed below -

public class clsGoldDiscountStrategy : IStrategy
    {
        public double calculate(double x, double y)
        {
            return (x * y) * 0.8;
        }
    }
}

concrete classes implementing the platinumstrategy is listed below -

public class clsPlatinumDiscountStrategy : IStrategy
    {
        public double calculate(double x, double y)
        {
            return (x * y) * 0.7;
        }
    }

The business logic to apply

public class clsBL
    {
        public double costPrice { get; set; }
        public double qty { get; set; }
        IStrategy _strategy;

        public clsBL(IStrategy strategy)
        {
            _strategy = strategy;
        }

        public double GetfinalPrice(double cp, double qty)
        {
            return _strategy.calculate(cp, qty); 
        }

    }

//Main method

static void Main(string[] args)
        {
            Console.WriteLine("Enter the discount Plan (Gold/Platinum)");
            string filter = Console.ReadLine().ToUpper();
            double result = 0;

            if (filter.Length > 0)
            {
                switch (filter)
                {
                    case "GOLD":
                        //Gold 
                        clsBL blgold = new clsBL(new clsGoldDiscountStrategy());
                        blgold.costPrice = 5;
                        blgold.qty = 10;

                        result = blgold.GetfinalPrice(blgold.costPrice, blgold.qty);
                        break;

                    case "PLATINUM":
                        //Platinum
                        clsBL blplatinum = new clsBL(new clsPlatinumDiscountStrategy());
                        blplatinum.costPrice = 10;
                        blplatinum.qty = 8;

                        result = blplatinum.GetfinalPrice(blplatinum.costPrice, blplatinum.qty);
                        break;
                    default:
                        Console.WriteLine("Enter the discount value as either  gold or platinum");
                        break;
                }

                Console.WriteLine("The result for " + filter + " is " + result);

            }
            else
            {
                Console.WriteLine("Enter the discount value");
                return;
            }


            Console.ReadLine();

        }
\$\endgroup\$
2
  • 1
    \$\begingroup\$ I have rolled back your last edit. Please see What should I do when someone answers my question?: Do not change the code in the question after receiving an answer. Incorporating advice from an answer into the question violates the question-and-answer nature of this site. \$\endgroup\$ – t3chb0t Oct 3 '18 at 5:40
  • \$\begingroup\$ sure. I will keep this in mind in future course of actions \$\endgroup\$ – krishna_v Oct 3 '18 at 5:41
5
\$\begingroup\$

The classes seem OK but the usage makes no sense:

                case "GOLD":
                    //Gold 
                    clsBL blgold = new clsBL(new clsGoldDiscountStrategy());
                    blgold.costPrice = 5;
                    blgold.qty = 10;

                    result = blgold.GetfinalPrice(blgold.costPrice, blgold.qty);
                    break;

This code (and the other switch case) does not use the polymorphism that the interface IStrategy enables. Rather, you can have simply written this code as:

                case "GOLD":
                    double GetGoldPrice(double x, double y) => (x * y) * 0.8;
                    result = GetGoldPrice(5, 10);
                    break;

Here, all classes and interfaces are deleted. There is just a static method.

If you want to use the strategy pattern it must look something like:

IStrategy strategy = GetStrategy();
strategy.Calculate(...);

Here, GetStrategy() can return any object compliant with that interface and further code does depend on any concrete type.


Regarding naming, the classes do not have to be called "Strategy". I'd use the names IDiscountCalculator and GoldDicountCalculator. Or, instead of "Calculator" you could use "Model" or "Provider".

\$\endgroup\$
4
  • \$\begingroup\$ how do we pass the parameters in this?(for cost price and qty). you mean to say we do not add dependency injection to the clBL class. could you kindly detail me \$\endgroup\$ – krishna_v Oct 1 '18 at 10:35
  • \$\begingroup\$ I depends on the way you want the parameters to behave. Maybe you should give the parameters to the strategy constructor. Then, the parameters are fixed for the lifetime of the strategy object. If the parameters are dynamic, then they should be parameters to the Calculate method. They should not be fields of settable properties on IStrategy. \$\endgroup\$ – usr Oct 1 '18 at 10:56
  • \$\begingroup\$ Well, they could be settable if you want a strategy to be changeable. Nothing wrong with that in principle... But immutable objects usually make for a cleaner API. \$\endgroup\$ – usr Oct 1 '18 at 10:58
  • \$\begingroup\$ @krishna_v there’s no sense in injecting dependencies you’re not using. As far as I can tell, you have properties that aren’t being used. \$\endgroup\$ – RubberDuck Oct 4 '18 at 23:39
5
\$\begingroup\$

x and y are really bad parameter names. I had to look at the client code to figure out that these were actually cost and quantity.

\$\endgroup\$
2
\$\begingroup\$
public double costPrice { get; set; }
public double qty { get; set; }

There is no point of having these two properties if you are passing those values to GetfinalPrice anyway:

public double GetfinalPrice(double cp, double qty)
{
    return _strategy.calculate(cp, qty); 
}

Pick one, otherwise it's pretty confusing.

\$\endgroup\$
2
  • \$\begingroup\$ I have modified the code. Now i am not sure how to pass the params to the gold and platinum strategy? I have hard coded the values. \$\endgroup\$ – krishna_v Oct 3 '18 at 5:35
  • \$\begingroup\$ @krishna_v I have modified the code. - this is not good because you must not change it after receiving answers... you need to undo the edit... or someone else will do it :-] \$\endgroup\$ – t3chb0t Oct 3 '18 at 5:37

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.