A better design pattern than factory?

Question

In the code I am now creating, I have an object that can belong to two discrete types, differentiated by serial number. Something like this:

class Chips {
public:
    Chips(int shelf) {m_nShelf = shelf;}
    Chips(string sSerial) {m_sSerial = sSerial;}
    virtual string GetFlavour() = 0;
    virtual int GetShelf() {return m_nShelf;}
protected:
    string m_sSerial;
    int m_nShelf;
}

class Lays : Chips {
    string GetFlavour() 
    {
        if (m_sSerial[0] == '0') return "Cool ranch";
        else return "";
    }
}

class Pringles : Chips {
    string GetFlavour() 
    {
        if (m_sSerial.find("cool") != -1) return "Cool ranch";
        else return "";
    }
}

Now, the obvious choice to implement this would be using a factory design pattern. Checking manually which serial belongs to which class type wouldn't be too difficult.

However, this requires having a class that knows all the other classes and refers to them by name, which is hardly truly generic, especially if I end up having to add a whole bunch of subclasses.

To complicate things further, I may have to keep around an object for a while before I know its actual serial number, which means I may have to write the base class full of dummy functions rather than keeping it abstract and somehow replace it with an instance of one of the child classes when I do get the serial. This is also less than ideal.

Is factory design pattern truly the best way to deal with this, or does anyone have a better idea?

Answer 1

Why bother with inheritance here? As far as I can see the behaviour is the same for all Chips instances. That behaviour is that the flavour is defined by the serial number.

If the serial number only changes a couple of things then you can inject or lookup the behaviours (std::function) at runtime based on the serial number using a simple map (why complicate things!). This way common behaviours are shared among different chips via their serial number mappings.

If the serial number changes a LOT of things, then I think you have the design a bit backwards. In that case what you really have is the serial number defining a configuration of the Chips, and your design should reflect that. Like this:

class SerialNumber {
public:
    // Maybe use a builder along with default values
    SerialNumber( .... ); 
    // All getters, no setters.
    string getFlavour() const;
private:
    string flavour;
    // others (package colour, price, promotion, target country etc...)
}

class Chips {
public:
    // Do not own the serial number... 'tis shared.
    Chips(std::shared_ptr<SerialNumber> poSerial):m_poSerial{poSerial}{} 
    Chips(int shelf, SerialNumber oSerial):m_poSerial{oSerial}, m_nShelf{shelf}{}
    string GetFlavour() {return m_poSerial->getFlavour()};
    int GetShelf() {return m_nShelf;}
protected:
    std::shared_ptr<SerialNumber> m_poSerial;
    int m_nShelf;
}

// stores std::shared_ptr but you could also use one of the shared containers from boost.
Chips pringles{ chipMap.at("standard pringles - sour cream") };

This way once you have a set of SerialNumbers for your products then the product behaviour does not change. The only change is the "configuration" which is encapsulated in the SerialNumber. Means that the Chips class doesn't need to change. Anyway, somewhere someone needs to know how to build the class. Of course you could you template based injection as well but your code would need to inject the correct type.

One last idea. If SerialNumber ctor took a string (XML or JSON for example) then you could have your program read the configurations at runtime, after they have been defined by a manager type person. This would decouple the business needs from your code, and that would be a robust way to future-proof.

Oh... and I would recommend NOT using Hungarian notation. If you change the type of an object or parameter you also have to change the name. Worse you could forget to change them and other will make incorrect assumptions. Unless you are using vim/notepad to program with then the IDE will give you that info in a clearer manner.

@user1158692 - The party instantiating Chips only needs to know about SerialNumber in one of my proposed designs, and that proposed design stipulates that the SerialNumber class acts to configure the Chips class. In that case the person using Chips SHOULD know about SerialNumber because of their intimate relationship. The intimiate relationship between the classes is exactly the reason why it should be injected via constructor. Of course it is very very simple to change this to use a setter instead if necessary, but this is something I would discourage, due to the represented relationship.

I really doubt that it is absolutely necessary to create the instances of chips without knowing the serial number. I would imagine that this is an application issue rather than one that is required by the design of the class. Also, the class is not very usable without SerialNumber and if you did allow construction of the class without SerialNumber you would either need to use a default version (requiring Chips to know how to construct one of these or using a global reference!) or you would end up polluting the class with a lot of checking.

As for you complaint regarding the shared_ptr... how on earth to you propose that the ownership semantics and responsibilities are clarified? Perhaps raw pointers would be your solution but that is dangerous and unclear. The shared_ptr clearly lets designers know that they do not own the pointer and are not responsible for it.

Answer 2

From the comments, I think you're after something like this:

    class ISerialNumber
    {
    public:
        static ISerialNumber* Create( const string& number )
        {
            // instantiate and return a concrete class that 
            // derives from ISerialNumber, or NULL
        }

        virtual void DoSerialNumberTypeStuff() = 0;
    };

    class SerialNumberedObject
    {
    public:
        bool Initialise( const string& serialNum ) 
        {
            m_pNumber = ISerialNumber::Create( serialNum );
            return m_pNumber != NULL;
        }

        void DoThings()
        {
            m_pNumber->DoSerialNumberTypeStuff();
        }

    private:
        ISerialNumber* m_pNumber;
    };

(As this was a question on more advanced concepts, protecting from null/invalid pointer issues is left as an exercise for the reader.)

Answer 3

You can create a factory which knows only the Base class, like this:

add pure virtual method to base class: virtual Chips* clone() const=0; and implement it for all derives, just like operator= but to return pointer to a new derived. (if you have destructor, it should be virtual too)

now you can define a factory class:

Class ChipsFactory{
  std::map<std::string,Chips*> m_chipsTypes;

public:
  ~ChipsFactory(){
     //delete all pointers... I'm assuming all are dynamically allocated.
     for( std::map<std::string,Chips*>::iterator it = m_chipsTypes.begin();
          it!=m_chipsTypes.end(); it++) {
        delete it->second;
     }
  }
  //use this method to init every type you have
  void AddChipsType(const std::string& serial, Chips* c){
    m_chipsTypes[serial] = c;
  }      
  //use this to generate object
  Chips* CreateObject(const std::string& serial){
    std::map<std::string,Chips*>::iterator it = m_chipsTypes.find(serial);
    if(it == m_chipsTypes.end()){
       return NULL;
    }else{
       return it->clone();
    }   
  }
};

Initialize the factory with all types, and you can get pointers for the initialized objects types from it.