Strategy pattern for four arithmetic operations

Question

I am studying design patterns, I was wondering if my approach makes sense. I am trying to implement the strategy pattern. I think I have captured the essence which is

• Define a family of algorithms,
• encapsulate each one,
• and make them interchangeable,

as shown in the example code below.

Have I captured the intent, am I missing something from the example?

#include "stdafx.h"
#include <iostream>

#include <iostream>
#include <fstream>
#include <string>

enum OperationType
{
    Add = 1, Subtract, Multiply, Divide
};

class Operation
{
public:
    Operation(int Value1, int Value2) :
        m_Value1(Value1),
        m_Value2(Value2) {      }

    virtual int Calculate() = 0;

private:

    int m_Result;
    int m_Value1;
    int m_Value2;

};

class Addition : public Operation
{
public:
    Addition(int Value1, int Value2) : Operation(Value1,Value2),
        m_Value1(Value1), m_Value2(Value2) {}

    int Calculate()
    {
        m_Result = m_Value1 + m_Value2;
        return m_Result;
    }

private:

    int m_Result;
    int m_Value1;
    int m_Value2;

};

class Subtraction : public Operation
{
public:
    Subtraction(int Value1, int Value2) : Operation(Value1, Value2),
        m_Value1(Value1), m_Value2(Value2) {}

    int Calculate()
    {
        m_Result = m_Value1 - m_Value2;
        return m_Result;
    }

private:

    int m_Result;
    int m_Value1;
    int m_Value2;

};

class Multiplication : public Operation
{
public:
    Multiplication(int Value1, int Value2) : Operation(Value1, Value2),
        m_Value1(Value1), m_Value2(Value2) {}

    int Calculate()
    {
        m_Result = m_Value1 * m_Value2;
        return m_Result;
    }

private:

    int m_Result;
    int m_Value1;
    int m_Value2;

};

class Division : public Operation
{
public:
    Division(int Value1, int Value2) : Operation(Value1, Value2),
        m_Value1(Value1), m_Value2(Value2) {}

    int Calculate()
    {
        m_Result = m_Value1 / m_Value2;
        return m_Result;
    }

private:

    int m_Result;
    int m_Value1;
    int m_Value2;

};


class Test
{
    public:     

        Test()
        {
            m_Operation = NULL;
        }

        void SetOperation(int Value1, int Value2, int Operation)
        {
            switch (Operation)
            {
                case Add:
                {
                    m_Operation = new Addition(Value1, Value2);
                }
                break;

                case Subtract:
                {
                    m_Operation = new Subtraction(Value1, Value2);
                }
                break;

                case Multiply:
                {
                    m_Operation = new Multiplication(Value1, Value2);
                }
                break;

                case Divide:
                {
                    m_Operation = new Division(Value1, Value2);
                }
                break;
            }
        }

        int PerformOperation()
        {
            return m_Operation->Calculate();
        }

private:
    Operation *m_Operation;

};



int main()
{
    Test test;

    test.SetOperation(1, 2, OperationType::Add);
    std::cout << test.PerformOperation() << std::endl;

    test.SetOperation(2, 3, OperationType::Subtract);
    std::cout << test.PerformOperation() << std::endl;

    test.SetOperation(3, 4, OperationType::Multiply);
    std::cout << test.PerformOperation() << std::endl;

    test.SetOperation(40, 10, OperationType::Divide);
    std::cout << test.PerformOperation() << std::endl;


    int x;
    std::cin >> x;
    return 0;
}

Answer 1

Overkill

This program is using dynamite to crack nuts. Choosing one operation of four is a very simple problem, easily handled with an enum and a switch. Indeed, your program already contains those, so the strategy classes are just unnecessary complexity.

I'm going to ignore that and review this program as just an exercise in using strategies. But if you were actually trying to solve a problem, this would be a horribly complex design.

Unfortunately, this makes it hard to judge how well you're using strategies, because you aren't using them for anything. It's also hard to advise how to simplify the program, because everything is unnecessary complexity.

How to use strategies

The Design Patterns movement is not a good guide to how to structure programs. Most of the techniques it teaches are rather specialized, and strategies are one of those. They're a useful tool to know, but they're only one tool of many, and you won't use them regularly.

When implementing strategies, you should prefer λ to custom classes. Fall back to classes only if λ isn't available or won't work — for example, if the strategy has more than one operation, or needs to be saved to a file.

Using λ is also a better way to learn to use strategies. If you're comfortable with first-class functions, and use them without thinking they're a Fancy Design Pattern, then strategies look like an obvious (but clumsy) variation, so it will be easy to use them correctly.

If you aren't already comfortable with λ and functional programming, that's far more important to learn than design patterns.

Repetition

Now for some actual code review.

The three private members of Operation (m_Result, m_Value1, m_Value2) are unused. And they're private, so the subclasses can't use them either, and must redefine them! If they're part of Operation's interface, they should be public or at least protected.

Unnecessary state

There's no reason for the strategies to store m_Value1, m_Value2 or m_Result. Calculate can simply take them as arguments and return the result. The interface could be like this:

class Operation {
    virtual int Calculate(int a, int b) = 0;
};

Similarly, Test::SetOperation modifies this for no good reason. Is this just to have an excuse to store an Operation somewhere? It could simply return an Operation, or it could perform the operation directly without storing it anywhere. Then there's no reason for Test to be a class at all. It could simply be a function.

C++

Don't make a habit of using std::endl: it flushes the stream, which is seldom what you want.

Test leaks its Operations. This is harmless in a toy program, but you should habitually use std::unique_ptr for owned data, so you don't have leaks when they matter.

Superficial details

I see from stdafx.h that you're writing in Microsoft style, so I won't complain about the m_ prefixes.

There are a lot of unnecessary blank lines. I don't know if these are also a deliberate stylistic feature, but they're usually thought to make the program harder to read, because you can't see as much of it at once.

I suppose the std::cin >> x at the end is a hack to make the program's window not disappear instantly? You shouldn't have to do this. If you're running the program from an IDE, it should have an option to fix this for you; if you run it from the command line, this problem should not arise.

A better strategy problem

If you'd like a better problem to practice using strategies, try implementing a game like fairy chess, where there are a wide variety of pieces with unique moves. Don't make a subclass for each one; instead, the constructor should take a strategy (represented as a function) to generate the piece's possible moves. The usage might look like this (with functions that return move generators):

Piece camel("camel", "L", Leaper(1, 3));
Piece rook("rook", "R", Rider(1, 0));

If that's too complicated or doesn't sound fun, try a simpler game with a variety of pieces or cards or actions, where each one takes a function (not a class) to calculate what it does.

Answer 2

You are potentially missing at least three points:

• You implement division, but its return type is int. Thus, 1/2 will not be returned as 0.5, which is worth observing at least.

• You are leaking memory: you call new but never deallocate the memory.

• Your base class does not provide a virtual destructor, meaning derived classes will not be destructed properly.

Some other points:

• Let me guess that you are compiling and running in a Windows environment. At the end of your main program, you don't have to explicitly return 0, and you might as well just do std::cin.get(); instead of reading to an int. But even better, assuming you use Visual Studio, is to go to "Properties > Linker > System" page, and set "SubSystem" to "Console". This keeps the console window open after the program has finished.

• As mentioned in the other review(s), your example does a lot of unnecessary work. A simpler implementation using what the standard provides of your program could be as follows:

  #include <iostream>
  #include <functional>
  
  template <typename Operation>
  int perform_operation(int x, int y)
  {
      return Operation()(x, y);
  }
  
  int main()
  {
      std::cout << perform_operation<std::plus<int> >(1, 2) << "\n";
      std::cout << perform_operation<std::minus<int> >(2, 3) << "\n";
      std::cout << perform_operation<std::multiplies<int> >(3, 4) << "\n";
      std::cout << perform_operation<std::divides<int> >(40, 10) << "\n";
  }
  

  This is the strategy pattern at work as well: every operation has a common interface, i.e., takes two parameters and returns one value. No need for dynamic memory allocations either.