2
\$\begingroup\$

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;
}
\$\endgroup\$
  • \$\begingroup\$ Abstract Factory is not a complicated pattern - but it appears complicated if you're just trying to use it for its own sake, rather than because you need one. It's one of those things where a good motivating example will really make things easier. Ask yourself whether you need a factory, and if so, whether you need an abstract one. \$\endgroup\$ – Toby Speight Jun 22 '17 at 16:00
  • \$\begingroup\$ Some good motivating examples are when you need to produce objects that provide common interface to resources of different types. For example, you might want to construct an audio processing pipeline from individual filters; all the objects returned will conform to an interface with one input and one output, but the program doesn't need to know the details as long as it can interact through that interface. Perhaps a more concrete example will help here? \$\endgroup\$ – Toby Speight Jun 22 '17 at 16:00
2
\$\begingroup\$

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.

\$\endgroup\$
  • \$\begingroup\$ The factory of factories seemed to be the point of the design pattern. In tutorialspoint.com/design_pattern/abstract_factory_pattern.htm the required shapes and colours are strings too, not enums., I think it is done to make the concept clearer. My CFactoryOfFactories represents FactoryProducer. In step 7 they show what seems a factory of factories. Right before step 7 the getShape method of the ColorFactory returns just null, which is in a way a half-implemented interface like the review sais. So I worry now: did I study a poor example or did I just misunderstand? ... \$\endgroup\$ – TradingDerivatives.eu Jun 28 '17 at 19:44
  • \$\begingroup\$ I've just followed your link, and I have mixed feelings about the material presented there. There's nothing actually wrong, but it's missing an explanation of why you might want to use an abstract factory and when it's a good solution to the problem. Don't discard it, but do read around and absorb some other ideas on the subject. Wikipedia's article, although only "start" class, is worthwhile reading. Usually, you only create a single factory (this could be from a factory of factories; more usually by code chosen at compile time). \$\endgroup\$ – Toby Speight Jun 28 '17 at 20:01
  • \$\begingroup\$ Once you have your factory, you normally hang on to that for the lifetime of your program (or at least as long as you continue obtaining objects). I think it's hard in this example to see why Color and Shape need to be produced by factories - in real-world examples, it's more evident why they can't be simply constructed (e.g. the concrete classes abstract different database, GUI or communication libraries). \$\endgroup\$ – Toby Speight Jun 28 '17 at 20:04
  • \$\begingroup\$ newthinktank.com/2012/09/abstract-factory-design-pattern has something too. What do you think about it? It also uses strings as arguments. The names of the variables, members and classes are not self-explanatory IMHO. \$\endgroup\$ – TradingDerivatives.eu Jun 28 '17 at 20:16

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.