Abstract Factory Design Pattern

Question

I have tried to implement the abstract factory design pattern. This is a complicated pattern. Can you review? My understanding of the pattern is: the objects to be constructed have two or more member object properties (here color and shape), each of which are created by a factory. The final objects themselves are also created by a factory, so we need a factory of factories. The objects and the member object properties are abstracted.

Compiling:

g++ -Wall -g -std=c++1y -Wall  -c /mnt/home/Data_MaOt/Short_C_and_Cpp_progs/DesignPatterns/04_AbstractFactoryShapesColors/CBlue.cpp -o obj/Debug/CBlue.o
g++ -Wall -g -std=c++1y -Wall  -c /mnt/home/Data_MaOt/Short_C_and_Cpp_progs/DesignPatterns/04_AbstractFactoryShapesColors/CCircle.cpp -o obj/Debug/CCircle.o
g++ -Wall -g -std=c++1y -Wall  -c /mnt/home/Data_MaOt/Short_C_and_Cpp_progs/DesignPatterns/04_AbstractFactoryShapesColors/CColoredShape.cpp -o obj/Debug/CColoredShape.o
g++ -Wall -g -std=c++1y -Wall  -c /mnt/home/Data_MaOt/Short_C_and_Cpp_progs/DesignPatterns/04_AbstractFactoryShapesColors/CColorFactory.cpp -o obj/Debug/CColorFactory.o
g++ -Wall -g -std=c++1y -Wall  -c /mnt/home/Data_MaOt/Short_C_and_Cpp_progs/DesignPatterns/04_AbstractFactoryShapesColors/CFactoryOfFactories.cpp -o obj/Debug/CFactoryOfFactories.o
g++ -Wall -g -std=c++1y -Wall  -c /mnt/home/Data_MaOt/Short_C_and_Cpp_progs/DesignPatterns/04_AbstractFactoryShapesColors/CGreen.cpp -o obj/Debug/CGreen.o
g++ -Wall -g -std=c++1y -Wall  -c /mnt/home/Data_MaOt/Short_C_and_Cpp_progs/DesignPatterns/04_AbstractFactoryShapesColors/CRectangle.cpp -o obj/Debug/CRectangle.o
g++ -Wall -g -std=c++1y -Wall  -c /mnt/home/Data_MaOt/Short_C_and_Cpp_progs/DesignPatterns/04_AbstractFactoryShapesColors/CRed.cpp -o obj/Debug/CRed.o
g++ -Wall -g -std=c++1y -Wall  -c /mnt/home/Data_MaOt/Short_C_and_Cpp_progs/DesignPatterns/04_AbstractFactoryShapesColors/CShapeFactory.cpp -o obj/Debug/CShapeFactory.o
g++ -Wall -g -std=c++1y -Wall  -c /mnt/home/Data_MaOt/Short_C_and_Cpp_progs/DesignPatterns/04_AbstractFactoryShapesColors/CTriangle.cpp -o obj/Debug/CTriangle.o
g++ -Wall -g -std=c++1y -Wall  -c /mnt/home/Data_MaOt/Short_C_and_Cpp_progs/DesignPatterns/04_AbstractFactoryShapesColors/main.cpp -o obj/Debug/main.o
g++  -o bin/Debug/04_AbstractFactoryShapesColors obj/Debug/CBlue.o obj/Debug/CCircle.o obj/Debug/CColoredShape.o obj/Debug/CColorFactory.o obj/Debug/CFactoryOfFactories.o obj/Debug/CGreen.o obj/Debug/CRectangle.o obj/Debug/CRed.o obj/Debug/CShapeFactory.o obj/Debug/CTriangle.o obj/Debug/main.o   
Output file is bin/Debug/04_AbstractFactoryShapesColors with size 351,28 KB
Process terminated with status 0 (0 minute(s), 9 second(s))
0 error(s), 0 warning(s) (0 minute(s), 9 second(s))

Output

int main(): Started.
void CColoredShape::Identify() const: I am a red circle.
void CColoredShape::Identify() const: I am a blue triangle.
void CColoredShape::Identify() const: I am a green rectangle.
int main(): Done.

CColorBase.h

#ifndef CCOLORBASE_H
#define CCOLORBASE_H

#include <string.h>

class CColorBase
{
public:
  virtual const std::string GetColor() const noexcept = 0;
};

#endif // CCOLORBASE_H

CFactoryBase.h

#ifndef CFACTORYBASE_H
#define CFACTORYBASE_H

#include <memory>

#include "string"

#include "CColorBase.h"
#include "CShapeBase.h"

class CFactoryBase
{
public:
  virtual std::unique_ptr<CColorBase> GetColor(const std::string aColor) const noexcept = 0;
  virtual std::unique_ptr<CShapeBase> GetShape(const std::string aShape) const noexcept = 0;
};

#endif // CFACTORYBASE_H

CShapeBase.h

#ifndef CSHAPEBASE_H
#define CSHAPEBASE_H

#include "string"

class CShapeBase
{
public:
  virtual const std::string GetShape() const noexcept = 0;
};

#endif // CSHAPEBASE_H

CBlue.cpp

#include <iostream>

#include "CBlue.h"

const std::string CBlue::GetColor() const noexcept
{
  return std::string("blue");
}

CBlue.h

#include "CColorBase.h"

class CBlue : public CColorBase
{
public:
  const std::string GetColor() const noexcept override;
};

CCircle.cpp

#include <iostream>

#include "CCircle.h"

const std::string CCircle::GetShape() const noexcept
{
  return std::string("circle");
}

CCircle.h

#ifndef CCIRCLE_H
#define CCIRCLE_H

#include "CShapeBase.h"

class CCircle : public CShapeBase
{
public:
  const std::string GetShape() const noexcept override;
};

#endif // CCIRCLE_H

CColorFactory.cpp

#include <iostream>

#include "CColorFactory.h"

#include "CRed.h"
#include "CGreen.h"
#include "CBlue.h"

std::unique_ptr<CShapeBase> CColorFactory::GetShape(std::string aShapeString) const noexcept
{
  std::cout << __PRETTY_FUNCTION__ << ": You should not call this method." << std::endl;
  exit(-1);
}

std::unique_ptr<CColorBase> CColorFactory::GetColor(std::string aColorString) const noexcept
{
  std::unique_ptr<CColorBase> myColorBase = nullptr;

  if (aColorString.compare("Red") == 0)
  {
    myColorBase.reset(new CRed());
  }
  else if (aColorString.compare("Blue") == 0)
  {
    myColorBase.reset(new CBlue());
  }
  else if (aColorString.compare("Green") == 0)
  {
    myColorBase.reset(new CGreen());
  }
  else
  {
    std::cout << __PRETTY_FUNCTION__ << ": Unknown color " << aColorString << std::endl;
    exit(-1);
  }

  return myColorBase;
}

CColorFactory.h

#ifndef CCOLORFACTORY_H
#define CCOLORFACTORY_H

#include <memory>

#include "CFactoryBase.h"

class CColorFactory : public CFactoryBase
{
public:
  std::unique_ptr<CShapeBase> GetShape(const std::string aShapeString) const noexcept override;
  std::unique_ptr<CColorBase> GetColor(const std::string aColorString) const noexcept override;
};

#endif // CCOLORFACTORY_H

CColoredShape.cpp

#include <iostream>

#include "CColoredShape.h"

#include "CFactoryBase.h"
#include "CFactoryOfFactories.h"

CColoredShape::CColoredShape(const std::string& aShape, const std::string& aColor) noexcept
{
  std::unique_ptr<CFactoryBase> shapeFactory = nullptr;
  std::unique_ptr<CFactoryBase> colorFactory = nullptr;
  std::unique_ptr<CShapeBase>   shape        = nullptr;
  std::unique_ptr<CColorBase>   color        = nullptr;

  shapeFactory = CFactoryOfFactories::GetFactory("Shape");
  colorFactory = CFactoryOfFactories::GetFactory("Color");

  iShape = shapeFactory->GetShape(aShape);
  iColor = colorFactory->GetColor(aColor);
}

void CColoredShape::Identify() const noexcept
{
  std::cout << __PRETTY_FUNCTION__ << ": I am a " << iColor->GetColor() << " " << iShape->GetShape() << "." << std::endl;
}

CColoredShape.h

#ifndef CCOLOREDSHAPE_H
#define CCOLOREDSHAPE_H

#include <string>
#include <memory>

#include "CColorBase.h"
#include "CShapeBase.h"

class CColoredShape
{
public:
  CColoredShape(const std::string& aShape, const std::string& aColor) noexcept;
  void Identify() const noexcept;

private:
  std::unique_ptr<CColorBase> iColor;
  std::unique_ptr<CShapeBase> iShape;
};


#endif // CCOLOREDSHAPE_H

CFactoryOfFactories.cpp

#include <iostream>

#include "CFactoryOfFactories.h"

#include "CShapeFactory.h"
#include "CColorFactory.h"

std::unique_ptr<CFactoryBase> CFactoryOfFactories::GetFactory(std::string aFactoryString)
{
  std::unique_ptr<CFactoryBase> myFactoryBase = nullptr;

  if (aFactoryString.compare("Shape") == 0)
  {
    myFactoryBase.reset(new CShapeFactory());
  }
  else if (aFactoryString.compare("Color") == 0)
  {
    myFactoryBase.reset(new CColorFactory());
  }
  else
  {
    std::cout << __PRETTY_FUNCTION__ << ": Unkown factory " << aFactoryString << std::endl;
    exit(-1);
  }

  return myFactoryBase;
}

CFactoryOfFactories.h

#ifndef CFACTORYOFFACTORIES_H
#define CFACTORYOFFACTORIES_H

#include <memory>

#include "CFactoryBase.h"

class CFactoryOfFactories
{
public:
  static std::unique_ptr<CFactoryBase> GetFactory(const std::string aFactoryString);
};


#endif // CFACTORYOFFACTORIES_H

CGreen.cpp

#include <iostream>

#include "CGreen.h"

const std::string CGreen::GetColor() const noexcept
{
  return std::string("green");
}

CGreen.h

#include "CColorBase.h"

class CGreen : public CColorBase
{
public:
  const std::string GetColor() const noexcept override;
};

CRectangle.cpp

#include <iostream>

#include "CRectangle.h"

const std::string CRectangle::GetShape() const noexcept
{
  return std::string("rectangle");
}

CRectangle.h

#ifndef CRECTANGLE_H
#define CRECTANGLE_H

#include "CShapeBase.h"

class CRectangle : public CShapeBase
{
public:
  const std::string GetShape() const noexcept override;
};

#endif // CRECTANGLE_H

CRed.cpp

#include <iostream>

#include "CRed.h"

const std::string CRed::GetColor() const noexcept
{
  return std::string("red");
}

CRed.h

#include "CColorBase.h"

class CRed : public CColorBase
{
public:
  const std::string GetColor() const noexcept override;
};

CShapeFactory.cpp

#include "iostream"

#include "CShapeFactory.h"

#include "CCircle.h"
#include "CTriangle.h"
#include "CRectangle.h"

std::unique_ptr<CShapeBase> CShapeFactory::GetShape(std::string aShapeString) const noexcept
{
  std::unique_ptr<CShapeBase> shapePtr = nullptr;

  if (aShapeString.compare("Circle") == 0)
  {
    shapePtr.reset(new CCircle());
  }
  else if (aShapeString.compare("Rectangle") == 0)
  {
    shapePtr.reset(new CRectangle());
  }
  else if (aShapeString.compare("Triangle") ==0)
  {
    shapePtr.reset(new CTriangle());
  }
  else
  {
    std::cout << __PRETTY_FUNCTION__ << ": Unknown shape " << aShapeString << std::endl;
    exit(-1);
  }

  return shapePtr;
}

std::unique_ptr<CColorBase> CShapeFactory::GetColor(std::string aColorString) const noexcept
{
  std::cout << __PRETTY_FUNCTION__ << ": You should not call this method." << std::endl;
  exit(-1);
}

CShapeFactory.h

#ifndef CSHAPEFACTORY_H
#define CSHAPEFACTORY_H

#include <memory>

#include "CFactoryBase.h"

class CShapeFactory : public CFactoryBase
{
private:
  std::unique_ptr<CShapeBase> GetShape(const std::string aShapeString) const noexcept override;
  std::unique_ptr<CColorBase> GetColor(const std::string aColorString) const noexcept override;
};


#endif // CSHAPEFACTORY_H

CTriangle.cpp

#include <iostream>

#include "CTriangle.h"

const std::string CTriangle::GetShape() const noexcept
{
  return std::string("triangle");
}

CTriangle.h

#ifndef CTRIANGLE_H
#define CTRIANGLE_H

#include "CShapeBase.h"

class CTriangle : public CShapeBase
{
public:
  const std::string GetShape() const noexcept override;
};

#endif // CTRIANGLE_H

main.cpp

#include <iostream>

#include "CColoredShape.h"

// Abstract factory design pattern
// Based on https://www.tutorialspoint.com/design_pattern/abstract_factory_pattern.htm

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

  CColoredShape redCircle = CColoredShape("Circle", "Red");
  CColoredShape blueTriangle = CColoredShape("Triangle", "Blue");
  CColoredShape greenRectangle = CColoredShape("Rectangle", "Green");

  redCircle.Identify();
  blueTriangle.Identify();
  greenRectangle.Identify();

  std::cout << __PRETTY_FUNCTION__ << ": Done." << std::endl;
}

Answer 1

Includes

The includes for std::string seem confused:

#include <string.h>

That's the C null-terminated string library, not std::string.

#include "string"

That may find a local string header instead of the installed library.

Both of those should be written as

#include <string>

C string literals convert to std::string

Instead of writing

const std::string CBlue::GetColor() const noexcept
{
  return std::string("blue");
}

You can take advantage of the provided conversion from const char* to std::string:

const std::string CBlue::GetColor() const
{
  return "blue";
}

It can't be noexcept, as constructing a std::string is permitted to throw. That means that you must either change the base class to allow exceptions, or find a way of returning a string without constructing it.

Don't half-implement interfaces

This is a pungent code-smell:

std::unique_ptr<CShapeBase> CColorFactory::GetShape(std::string aShapeString) const noexcept
{
  std::cout << __PRETTY_FUNCTION__ << ": You should not call this method." << std::endl;
  exit(-1);
}

This suggests to me that the CFactoryBase interface should probably be two independent interfaces. You should never be required by an interface to write a method like that; you want to discover such an error when compiling, rather than run the risk of a serious bug escaping because it's not detected until all the stars are in alignment!

What's a "factory of factories"?

The previous point hints at a design problem, and the CFactoryOfFactories confirms it. It's rare that you need to construct a factory by name without knowing its kind. The point of an abstract factory is that you can decide at run-time which implementation of a given kind of factory will be used to construct new objects. So you might have a shape factory that produces instrumented shapes for tracing or one that produces highly-optimised shapes for performance. You don't generally want to specify by name which kind of factory you need. So I'd expect something more like

class AbstractFactoryCreator
{
     std::shared_ptr<AbstractShapeFactory> get_shape_factory() const;
     std::shared_ptr<AbstractColorFactory> get_color_factory() const;
}

It may also be worth revisiting the naming of methods, being clearer about what's a get() of something that may already exist and what's a create() that always constructs a new object.

Don't embed standard streams deeply in your classes.

I know that Indentify() is only there to demonstrate your factory, but as a general principle, avoid using cout, cerr, or cin in library code; prefer (in this order, best first):

• std::ostream& operator<<(std::ostream&, const Type&) free function
• std::ostream& operator<<(std::ostream&) const member
• std::string to_string() const member or a conversion operator (perhaps used by the free function above)

Prefer initializers to assignment

This code assigns each pointer twice - once to construct it and once by assignment:

CColoredShape::CColoredShape(const std::string& aShape, const std::string& aColor) noexcept
{
  std::unique_ptr<CFactoryBase> shapeFactory = nullptr;
  std::unique_ptr<CFactoryBase> colorFactory = nullptr;
  std::unique_ptr<CShapeBase>   shape        = nullptr;
  std::unique_ptr<CColorBase>   color        = nullptr;

Instead, you could construct each one properly:

CColoredShape::CColoredShape(const std::string& aShape, const std::string& aColor) noexcept
  : shapeFactory{},
    colorFactory{},
    color{},
    shape{}
{

Or even easier, just give defaults in the class definition:

  std::unique_ptr<CColorBase> color = {};
  std::unique_ptr<CShapeBase> shape = {};

Use == for equality comparison

Tests such as:

  if (aShapeString.compare("Circle") == 0)

seem obfuscated compared with the more obvious:

  if (aShapeString == "Circle")

As a rule, only use std::string::compare() if you are using all three possible return values (positive, negative, zero).

Enable more warnings

You're only using -Wall; I'd recommend also -Wextra and possibly -Weffc++ to highlight your design issues.