2
\$\begingroup\$

I am going over a number of design patterns and today I ask you to please inspect my application attempt of the Factory design pattern. Please feel free to share what you think about it, because my goal is to improve my skills. I am using g++ 4.8.5. Note: this is not homework, just practice.

In short, several classes are derived from CShapeBase and these are produced by CFactory.cpp and returned to main.cpp; main.cpp is not aware of which class is instantiated, which appears to be the purpose of the factory design pattern.

Compiling:

g++ -Wall -fexceptions -g -std=c++11  -c /mnt/home/Data_MaOt/Short_C_and_Cpp_progs/DesignPatterns/03_Factory_Pattern/CCircle.cpp -o obj/Debug/CCircle.o
g++ -Wall -fexceptions -g -std=c++11  -c /mnt/home/Data_MaOt/Short_C_and_Cpp_progs/DesignPatterns/03_Factory_Pattern/CFactory.cpp -o obj/Debug/CFactory.o
g++ -Wall -fexceptions -g -std=c++11  -c /mnt/home/Data_MaOt/Short_C_and_Cpp_progs/DesignPatterns/03_Factory_Pattern/CHexagon.cpp -o obj/Debug/CHexagon.o
g++ -Wall -fexceptions -g -std=c++11  -c /mnt/home/Data_MaOt/Short_C_and_Cpp_progs/DesignPatterns/03_Factory_Pattern/CPentagon.cpp -o obj/Debug/CPentagon.o
g++ -Wall -fexceptions -g -std=c++11  -c /mnt/home/Data_MaOt/Short_C_and_Cpp_progs/DesignPatterns/03_Factory_Pattern/CRectangle.cpp -o obj/Debug/CRectangle.o
g++ -Wall -fexceptions -g -std=c++11  -c /mnt/home/Data_MaOt/Short_C_and_Cpp_progs/DesignPatterns/03_Factory_Pattern/CSquare.cpp -o obj/Debug/CSquare.o
g++ -Wall -fexceptions -g -std=c++11  -c /mnt/home/Data_MaOt/Short_C_and_Cpp_progs/DesignPatterns/03_Factory_Pattern/main.cpp -o obj/Debug/main.o
g++  -o bin/Debug/03_Factory_Pattern obj/Debug/CCircle.o obj/Debug/CFactory.o obj/Debug/CHexagon.o obj/Debug/CPentagon.o obj/Debug/CRectangle.o obj/Debug/CSquare.o obj/Debug/main.o

CCircle.cpp

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

CCircle::CCircle()
{
  std::cout << "CCircle::CCircle(): Started and finished." << std::endl;
}

void CCircle::Identify()
{
  std::cout << "CCircle::CCircle(): I am a CCircle object." << std::endl;
}

CCircle.h

#ifndef CCIRCLE_H
#define CCIRCLE_H

#include "CShapeBase.h"

class CCircle : public CShapeBase
{
public:
  CCircle();
  void Identify();
};

#endif // CCIRCLE_H

CFactory.cpp

#include <iostream>
#include <memory>

#include "CFactory.h"

#include "CCircle.h"
#include "CSquare.h"
#include "CRectangle.h"
#include "CPentagon.h"
#include "CHexagon.h"

std::shared_ptr<CShapeBase> CFactory::Produce()
{
  std::shared_ptr<CShapeBase> myBasePtr = nullptr;

  while(myBasePtr == nullptr)
  {
    std::cout << "Please select your shape:" << std::endl;
    std::cout << "1) Circle" << std::endl;
    std::cout << "2) Square" << std::endl;
    std::cout << "3) Rectangle" << std::endl;
    std::cout << "4) Pentagon" << std::endl;
    std::cout << "5) Hexagon" << std::endl;

    int input = 0;
    std::cin >> input;

    switch(input)
    {
    case 1:
      myBasePtr = std::make_shared<CCircle>();
      break;
    case 2:
      myBasePtr = std::make_shared<CSquare>();
      break;
    case 3:
      myBasePtr = std::make_shared<CRectangle>();
      break;
    case 4:
      myBasePtr = std::make_shared<CPentagon>();
      break;
    case 5:
      myBasePtr = std::make_shared<CHexagon>();
      break;
    default:
      std::cout << "CFactory::Produce(): Did not produce a shape." << std::endl;
      break;
    }
  }

  return myBasePtr;
}

CFactory.h

#ifndef CFACTORY_H
#define CFACTORY_H

#include <memory>

#include "CShapeBase.h"

// Include derives classes here?

class CFactory
{
public:
  std::shared_ptr<CShapeBase> Produce();
};

#endif // CFACTORY_H

CHexagon.cpp

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

CHexagon::CHexagon()
{
  std::cout << "CHexagon::CHexagon(): Started and finished." << std::endl;
}

void CHexagon::Identify()
{
  std::cout << "CHexagon::CHexagon(): I am a CHexagon object." << std::endl;
}

CHexagon.h

#ifndef CHEXAGON_H
#define CHEXAGON_H

#include "CShapeBase.h"

class CHexagon : public CShapeBase
{
public:
  CHexagon();
  void Identify();
};

#endif // CHEXAGON_H

CPentagon.cpp

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

CPentagon::CPentagon()
{
  std::cout << "CPentagon::CPentagon(): Started and finished." << std::endl;
}

void CPentagon::Identify()
{
  std::cout << "CPentagon::CPentagon(): I am a CPentagon object." << std::endl;
}

CPentagon.h

#ifndef CPENTAGON_H
#define CPENTAGON_H

#include "CShapeBase.h"

class CPentagon : public CShapeBase
{
public:
  CPentagon();
  void Identify();
};

#endif // CPENTAGON_H

CRectangle.cpp

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

CRectangle::CRectangle()
{
  std::cout << "CRectangle::CRectangle(): Started and finished." << std::endl;
}

void CRectangle::Identify()
{
  std::cout << "CRectangle::CRectangle(): I am a CRectangle object." << std::endl;
}

CRectangle.h

#ifndef CRECTANGLE_H
#define CRECTANGLE_H

#include "CShapeBase.h"

class CRectangle : public CShapeBase
{
public:
  CRectangle();
  void Identify();
};

#endif // CRECTANGLE_H

CShapeBase.h

#ifndef CSHAPEBASE_H
#define CSHAPEBASE_H

class CShapeBase
{
public:
  virtual void Identify() = 0;
};

#endif // CSHAPEBASE_H

CSquare.cpp

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

CSquare::CSquare()
{
  std::cout << "CSquare::CSquare(): Started and finished." << std::endl;
}

void CSquare::Identify()
{
  std::cout << "CSquare::CSquare(): I am a CSquare object." << std::endl;
}

CSquare.h

#ifndef CSHAPE_H
#define CSHAPE_H

#include "CShapeBase.h"

class CSquare : public CShapeBase
{
public:
  CSquare();
  void Identify();
};

#endif // CSHAPE_H

main.cpp

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

int main()
{
  std::cout << "main(): Started." << std::endl;

  CFactory myFactory;

  std::shared_ptr<CShapeBase> myBasePtr = myFactory.Produce();

  myBasePtr->Identify();

  std::cout << "main(): Finished." << std::endl;
  return 0;
}
\$\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\$ – Peilonrayz Apr 3 '17 at 14:15
6
\$\begingroup\$

Sure. You implemented the basic factory pattern.
But factories don't get useful to you start using Abstract Factories. SO you can swap out out factory for another factory.

Side Note: When presenting code it's easier to review of the header comes before the source. And best if the base class comes first. Prevents a lot of scrolling backwards and forwards.

Virtual Functions

class CShapeBase
{
public:
  virtual void Identify() = 0;
};

If a class has a virtual function and it will be dynamically created and destroyed via a pointer.

CShapeBase*  object = new Circle();
...             // Note: Demo use smart pointers in reality.
delete object;

Then you should also have a virtual destructor. As it currently stands your code will call the destructor of CShapeBase but because the destructor is not virtual it will not call the destructor of Circle.

Overiding virtual functions

When you override a virtual function its a good idea to mark it as overridden. That way if things in the future change your compiler will catch any changes and prevent compilation until you fix the update.

class CSquare : public CShapeBase
{
public:
  CSquare();
  void Identify() override;
       //         ^^^^^^^^
};

Ownership

 std::shared_ptr<CShapeBase> CFactory::Produce()

Are you sure you want a shared_ptr? The factory is not retaining ownership so there is no dual ownership.

Personally I would return unique_ptr. The user of the code can then decide if they want to upgrade from unique_ptr to shared_ptr if that's what they require (as you can assign a unique_ptr to a shared_ptr.

The reason I would use unique_ptr is that there is basically no overhead from unique_ptr while shared_ptr has a significant overhead because of the need to track weak_ptr internally.

\$\endgroup\$

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.