Good factory ​implementat​ion ? (C++)

I'm trying to implement a factory method in our application. Do you thing that this kind of implementation is a good one ?

class Person
{
public:
   Person(PersonData personData);
   ...

private:
   int m_identifier;
   std::string m_name;
};

class Employee : public Person
{
public:
   Employee(PersonData personData);
   ...

private:
   float m_hourlyWage;
};

class Boss : public Person
{
public:
   Boss(PersonData personData);
   ...

private:
   float m_annualSalary;
   float m_bonus;
};

struct PersonData
{
   int type;
   int identifier;
   std::string name;
   float hourlyWage;
   float annualSalary;
   float bonus;
};

class Factory
{
public:
   static Person* CreatePerson(PersonData personData);
};

Person* Factory::CreatePerson(PersonData personData)
{
   switch (personData.type)
   {
      case PersonType_Employee: 
         return new Employee(personData);
         break;

      case PersonType_Boss: 
         return new Boss(personData);
         break;
   
      default:
         throw std::invalid_argument("...");
         break;
   }
}

Well, you've created classes which can be instantiated on their own, so no-one needs to go through the factory. (I always take the "don't let them break the rules, even if they should know better" option.)

So hiding the constructor and making the factory a friend class (or analogous, my C++ is getting rusty) would force people to use the factory to generate instances.

I may even have been tempted to have a static "CreateInstance" method on the person classes which invokes the factory behind the scenes to create the object. But then it kinda depends on what you're using the Factory for as to whether that would be even half a sensible idea.

The goal is to have a method that read the data from a DB or file that look like this :

std::vector<Person*> ReadStreamFromFile()
{
   ...

   PersonData personData;

   while ( ReadNext(personData) )
   {
      Person* person = Factory::CreatePerson(personData);
      persons.push_back(person);
   }

   return persons;
}

What you've got works in that case. I usually see factories used as a means to enforce certain policies on the numbers / types of objects created, so allowing people to create objects outside the factory is a bad thing in those cases. In your case it probably wouldn't matter.

Having a factory class which just houses a single static method seems OTT, but the alternative (putting the static method in the Person class) offends my sense that inheritance ought to be one-way, and that parents shouldn't know about what their children are up to. (This also applies to real life.)

Massif wrote:

Having a factory class which just houses a single static method seems OTT

private or internal ctors in .NET don't really offer any 'protection' as such, only an obsfucation..

In which case its seen that a factory is construction and initialization of a type.. imagine having an interface where you didn't want a ctor to do the 'same' thing on each class and equally the type of ctor you wanted wasn't tied 1 : 1 with that interface (ie, you would need another set of ctors to handle another type of usage)..

The factory then lets you abstract this initialization away which can help with versioning problems etc etc..

At least; thats how I've seen a factory anyway..

Edit: Factorys also lend the ability to generate 'creation' caching stategy as well (such as being able to skip creation of if its legal that you can return a previously created one (based on your heuristics) which can be held internally to the factory)

stevo_ wrote:



private or internal ctors in .NET don't really offer any 'protection' as such, only an obsfucation..

I agree, but it does mean you shouldn't take for granted the ctor won't be called.. in most cases you'll probably just cause an exception, but its up to you as the implementor of the system to ensure you don't cause data corruption..

It's also very easy for someone to call any ctor they wish using a single param overload on the activator.. in some cases; systems designed for generic activation of classes could be calling into private or internal ctors.. (serializers come to mind).

stevo_ wrote:

I agree, but it does mean you shouldn't take for granted the ctor won't be called.. in most cases you'll probably just cause an exception, but its up to you as the implementor of the system to ensure you don't cause data corruption..

It's also very easy for someone to call any ctor they wish using a single param overload on the activator.. in some cases; systems designed for generic activation of classes could be calling into private or internal ctors.. (serializers come to mind).

Yes I'm aware of what a singleton is however, being a singleton doesn't nec~ mean you cannot be created twice in .NET, although it does allow you to design your ctor to throw exceptions based upon static 'initialized' flags..

However, you may not always want your classes to be singletons.. so picking a singleton pattern to satisfy creation 'policing' isn't a logical action.

The point of the factory pattern is not to restrict access to creating the objects without the factory.  It is simply to allow the creation of objects without specifying the class of the object you want to create.

The implementation you provided was just fine.