6

BIG EDIT

So after gathering some feedback from all of you, and meditating on the XY problem as Zack suggested, I decided to add another code example which illustrates exactly what I'm trying to accomplish (ie the "X") instead of asking about my "Y".


So now we are working with cars and I've added 5 abstract classes: ICar, ICarFeatures, ICarParts, ICarMaker, ICarFixer. All of these interfaces will wrap or use a technology-specific complex object provided by a 3rd party library, depending on the derived class behind the interface. These interfaces will intelligently manage the life cycle of the complex library objects.

My use case here is the FordCar class. In this example, I used the Ford library to access classes FordFeatureImpl, FordPartsImpl, and FordCarImpl. Here is the code:

class ICar {
public:
    ICar(void) {}
    virtual ~ICar(void) {}
};

class FordCar : public ICar {
public:
    ICar(void) {}
    ~FordCar(void) {}
    FordCarImpl* _carImpl;
};

class ICarFeatures {
public:
    ICarFeatures(void) {}
    virtual ~ICarFeatures(void) {}
    virtual void addFeature(UserInput feature) = 0;
};

class FordCarFeatures : public ICarFeatures{
public:
    FordCarFeatures(void) {}
    virtual ~FordCarFeatures(void) {}
    virtual void addFeature(UserInput feature){

        //extract useful information out of feature, ie:
        std::string name = feature.name;
        int value = feature.value;
        _fordFeature->specialAddFeatureMethod(name, value);
    }

    FordFeatureImpl* _fordFeature;
};

class ICarParts {
public:
    ICarParts(void) {}
    virtual ~ICarParts(void) {}
    virtual void addPart(UserInput part) = 0;
};

class FordCarParts :public ICarParts{
public:
    FordCarParts(void) {}
    virtual ~FordCarParts(void) {}
    virtual void addPart(UserInput part) {

        //extract useful information out of part, ie:
        std::string name = part.name;
        std::string dimensions = part.dimensions;
        _fordParts->specialAddPartMethod(name, dimensions);
    }
    FordPartsImpl* _fordParts;
};

class ICarMaker {
public:
    ICarMaker(void) {}
    virtual ~ICarMaker(void) {}
    virtual ICar* makeCar(ICarFeatures* features, ICarParts* parts) = 0;
};

class FordCarMaker {
public:
    FordCarMaker(void) {}
    virtual ~FordCarMaker(void) {}
    virtual ICar* makeCar(ICarFeatures* features, ICarParts* parts){

        FordFeatureImpl* fordFeatures = dynamic_cast<FordFeatureImpl*>(features);
        FordPartsImpl* fordParts = dynamic_cast<FordPartsImpl*>(parts);

        FordCar* fordCar = customFordMakerFunction(fordFeatures, fordParts);

        return dynamic_cast<ICar*>(fordCar);
    }

    FordCar* customFordMakerFunction(FordFeatureImpl* fordFeatures, FordPartsImpl* fordParts) {

        FordCar* fordCar =  new FordCar;

        fordCar->_carImpl->specialFeatureMethod(fordFeatures);
        fordCar->_carImpl->specialPartsMethod(fordParts);

        return fordCar;
    }
};


class ICarFixer {
public:
    ICarFixer(void) {}
    virtual ~ICarFixer(void) {}
    virtual void fixCar(ICar* car, ICarParts* parts) = 0;
};


class FordCarFixer {
public:
    FordCarFixer(void) {}
    virtual ~FordCarFixer(void) {}
    virtual void fixCar(ICar* car, ICarParts* parts) {

        FordCar* fordCar = dynamic_cast<FordCar*>(car);
        FordPartsImpl* fordParts = dynamic_cast<FordPartsImpl*>(parts);

        customFordFixerFunction(fordCar, fordParts);


}

customFordFixerFunction(FordCar* fordCar, FordPartsImpl* fordParts){
    fordCar->_carImpl->specialRepairMethod(fordParts);
}
};

Notice that I must use dynamic casting to access the technology-specific objects within the abstract interfaces. This is what makes me think I'm abusing inheritance and provoked me to ask this question originally.

Here is my ultimate goal:

UserInput userInput = getUserInput(); //just a configuration file ie XML/YAML
CarType carType = userInput.getCarType();

ICarParts* carParts = CarPartFactory::makeFrom(carType);
carParts->addPart(userInput);

ICarFeatures* carFeatures = CarFeaturesFactory::makeFrom(carType);
carFeatures->addFeature(userInput);

ICarMaker* carMaker = CarMakerFactory::makeFrom(carType);
ICar* car = carMaker->makeCar(carFeatures, carParts);

UserInput repairSpecs = getUserInput();
ICarParts* replacementParts = CarPartFactory::makeFrom(carType);
replacementParts->addPart(repairSpecs);

ICarFixer* carFixer = CarFixerFactory::makeFrom(carType);
carFixer->fixCar(car, replacementParts);

Perhaps now you all have a better understanding of what I'm trying to do and perhaps where I can improve.

I'm trying to use pointers of base classes to represent derived (ie Ford) classes, but the derived classes contain specific objects (ie FordPartsImpl) which are required by the other derived classes (ie FordCarFixer needs a FordCar and FordPartsImpl object). This requires me to use dynamic casting to downcast a pointer from the base to its respective derived class so I can access these specific Ford objects.

trianta2
  • 3,952
  • 5
  • 36
  • 52
  • Do you have an actual use case for this dynamic polymorphism? Or could you use concrete non-polymorphic types everywhere, with templates to make code generic? –  Feb 26 '14 at 19:59
  • My use case is that I'm making models from data, and I want to be able to instantiate the correct model analyzer from my model and then use the model analyzer to analyze future data. I have many different types of models, produced by different technologies, so I want to have a "Model" interface and "Model Analyzer" interface that I can swap in/out using dependency injection. I can always find the type of technology/model I'm using via the Visitor pattern. – trianta2 Feb 26 '14 at 20:03
  • "worker will dynamic_cast" — this sentence alone means you are, in fact, abusing inheritance. You also have not demonstrated any need to use inheritance. – n. m. could be an AI Feb 26 '14 at 20:06
  • As for what I can see from your sample a simple `static_cast<>()` would be sufficient, no need for `dynamic_cast<>()` IMHO. I'm not sure about your use case, but for me it looks like the hard thing may be to get the binding of `Object`s and `ObjectWorker`s right – πάντα ῥεῖ Feb 26 '14 at 20:10
  • @n.m. I believe I require inheritance because I want to abide by the open/close principle. I do not want to use overloading to expand the interface of ObjectWorker to accommodate many different types of objects, because if a new type of object is introduced, then I must now change ObjectWorker. If I use inheritance, I can derive and build a new ObjectWorker that is made specifically for the new type, and no existing classes need to be changed. – trianta2 Feb 26 '14 at 20:10
  • @trianta2 If the call site can know the concrete type, all code that needs to be generic over various models (i.e. would deal with `Object` and `ObjectWorker` pointers/references) can be template'd over the object and worker types. Then there would be no need for inheritance. I can't tell from a distance if this is feasible for your application and your existing code, but it does work in many cases and would offer several advantages. –  Feb 26 '14 at 20:16
  • @delnan I'm afraid I don't quite understand. Could you possibly explain with a code example? Do you mean something like this: http://stackoverflow.com/questions/7784898/dynamic-template-instantiation – trianta2 Feb 26 '14 at 20:27
  • There's no open/closed principle involved, as you don't extend any existing interface, except purely formally. `doWork` is never called on an `Object` pointer that points to something of statically-unknown type, which means `doWork` can be safely moved to subclasses of `Object`, which means the `Object` class loses its only reason to exist and can be safely killed. – n. m. could be an AI Feb 26 '14 at 20:30
  • @n.m. I'm still not getting your point. `StringObjectWorker` is extending an existing interface of `ObjectWorker`. The `Object` pointer is statically unknown; its type can and is dynamically created via factory pattern during run-time. – trianta2 Feb 26 '14 at 20:47
  • 2
    "its type can and is dynamically created via factory pattern during run-time" --- the details here are vitally important and you should not relegate them to comments. Posted code **as it stands right now** needs no inheritance. To discuss inheritance, post code (or even pseudocode) that does genuinely need it. The exact manner you implement inheritance depends on what exactly is needed. – n. m. could be an AI Feb 26 '14 at 21:02
  • @n.m. My apologies. I've updated my example code to better reflect my problem at hand. Factories have been included. – trianta2 Feb 26 '14 at 21:05
  • This sounds like it would call for the http://en.wikipedia.org/wiki/Bridge_pattern, but I'm still a little vague on what trianta2 is trying to accomplish. – Adrian McCarthy Feb 28 '14 at 23:52
  • Note: you don't have to specify `void` for parameterless methods in C++, in C all functions without parameters and without `void` were vararg functions but in C++ this isn't true, you can define your methods without `void`. – pasztorpisti Mar 01 '14 at 04:54

5 Answers5

4

My question is: am I abusing inheritance here? I'm trying to have a many-to-many relationship between the workers and objects. I feel like I'm doing something wrong by having an Object family of class which literally do nothing but hold data and making the ObjectWorker class have to dynamic_cast the object to access the insides.

That is not abusing inheritance... This is abusing inheritance

class CSNode:public CNode, public IMvcSubject, public CBaseLink,
         public CBaseVarObserver,public CBaseDataExchange, public CBaseVarOwner

Of which those who have a C prefix have huge implementations

Not only that... the Header is over 300 lines of declarations.

So no... you are not abusing inheritance right now.

But this class I just showed you is the product of erosion. I'm sure the Node as it began it was a shinning beacon of light and polymorphism, able to switch smartly between behavior and nodes.

Now it has become a Kraken, a Megamoth, Cthulu itself trying to chew my insides with only a vision of it.

Heed this free man, heed my counsel, beware of what your polymorphism may become.

Otherwise it is fine, a fine use of inheritance of something I suppose is an Architecture in diapers.

What other alternatives do I have if I want to only have a single work() method?

Single Work Method... You could try:

  • Policy Based Design, where a policy has the implementation of your model
  • A Function "work" that it is used by every single class
  • A Functor! Instantiated in every class that it will be used

But your inheritance seems right, a single method that everyone will be using.

One more thing....I'm just gonna leave this wiki link right here

Or maybe just copy paste the wiki C++ code... which is very similar to yours:

#include <iostream>
#include <string>
 
template <typename OutputPolicy, typename LanguagePolicy>
class HelloWorld : private OutputPolicy, private LanguagePolicy
{
    using OutputPolicy::print;
    using LanguagePolicy::message;
 
public:
    // Behaviour method
    void run() const
    {
        // Two policy methods
        print(message());
    }
};
 
class OutputPolicyWriteToCout
{
protected:
    template<typename MessageType>
    void print(MessageType const &message) const
    {
        std::cout << message << std::endl;
    }
};
 
class LanguagePolicyEnglish
{
protected:
    std::string message() const
    {
        return "Hello, World!";
    }
};
 
class LanguagePolicyGerman
{
protected:
    std::string message() const
    {
        return "Hallo Welt!";
    }
};
 
int main()
{
    /* Example 1 */
    typedef HelloWorld<OutputPolicyWriteToCout, LanguagePolicyEnglish> HelloWorldEnglish;
 
    HelloWorldEnglish hello_world;
    hello_world.run(); // prints "Hello, World!"
 
    /* Example 2 
     * Does the same, but uses another language policy */
    typedef HelloWorld<OutputPolicyWriteToCout, LanguagePolicyGerman> HelloWorldGerman;
 
    HelloWorldGerman hello_world2;
    hello_world2.run(); // prints "Hallo Welt!"
}

More important questions are

How are you going to use an Int Object with your StringWorker?

You current implementation won't be able to handle that

With policies it is possible.

What are the possible objects?

Helps you define if you need this kind of behavior

And remember, don't kill a chicken with a shotgun

Maybe your model will never really change overtime.

Community
  • 1
  • 1
Claudiordgz
  • 3,023
  • 1
  • 21
  • 48
  • Thank you for the thorough response. I will definitely look into Policy Based Design. To answer your questions, currently, an IntObject should never reach a StringWorker, as the Factory will always be getting the correct type via the Visitor pattern. If the dynamic_cast fails, I will be throwing an exception. I do like your shotgun and chicken line and will keep it in mind, but unfortunately the models themselves wrap complex models taken from 3rd party libraries, so as soon as the technology changes, the model changes drastically. – trianta2 Feb 26 '14 at 21:26
  • I'll be honest with you, I love policies, I have donde some behaviors that could have easily been done with argument passing or inheritance. But according to your explanation they just might be the thing for you, you can extend a policy as long as you want. Not only that, your policy don't necessarily have to inherit every method, just the ones you need from the Interface. So if your model changes, you could just use a new policy, define it as an adapter, and be done with it. You master class would still receive the data in the same format. – Claudiordgz Feb 26 '14 at 21:31
  • So I've done some reading on policy based design ("static polymorphism") and, if I understand correctly, it appears that different categories of policies (ie Output, Language in your example) must be orthogonal or functionally independent. In my problem this is not the case. In my new example, FordCarFixer requires a FordCar. It wouldn't make sense for a FordCarFixer to receive a BmwCar or MercedesCar object. I do not know if policy based design is the right solution for me, or am I still missing something? – trianta2 Mar 01 '14 at 14:21
  • I added some examples to your post of how would you use Policies and Templates to effectively write less code and debug in one place for all your Types of Car. – Claudiordgz Mar 01 '14 at 15:15
  • Is it possible to use policy based design dynamically? Ie can you mix-and-match different policies on the fly during run-time? Or would that fall under the domain of the Strategy pattern? – trianta2 Mar 05 '14 at 03:08
  • @trianta2 Depends, Strategy pattern is to change a method during runtime and replace it with another. You could mix and max different policies and manage them in a normal switch case, you could try a map of string->constructor that gets the input and returns an instance of what you need. I thought you had that from your code snippet that does CarPartFactory::makeFrom(carType); carType would most likely be received from an Enum, Map, or List of Function Pointers. You are already doing something FactoryStrategyshly patternishly here. Just keep construction in one part and definition in other. – Claudiordgz Mar 05 '14 at 06:01
  • So essentially you'll have to build each combination of policies for use in a factory? What happens when you add new policies? Do you need to recompile? Or can you still abide by the open-close principle? Do you have any factory implementations that you prefer which use templates like in Policy Based Design? I would prefer to have a string->object mapping. – trianta2 Mar 05 '14 at 19:37
  • Yes, the purpose of this it's compilation time. If you need to add further definitions in runtime you need to consider separating the implementations in a dll. A dll manager that checks a path in your system, and when a new dll appears instantiate the element (like a plugin system). This way you can add implementations in runtime. I don't have factory implementations in which I prefer to use templates though, I use them where I can code much less and test once. – Claudiordgz Mar 05 '14 at 22:04
  • I'm not sure if policies will solve my original issue, but you've definitely expanded my view of how I can design these classes, so I award you the bounty. I think perhaps I have chosen poor abstractions with these classes, and that another interface should be chosen. On an unrelated note, could you possibly check this other post I have since you appear to be very knowledgeable with policies: http://stackoverflow.com/questions/22240676/c-policy-based-design-with-variable-type-return-value :) – trianta2 Mar 07 '14 at 14:11
1

You have committed a design error, but it is not "abuse of inheritance". Your error is that you are trying to be too generic. Meditate upon the principle of You Aren't Gonna Need It. Then, think about what you actually have. You don't have Objects, you have Dogs, Cats, and Horses. Or perhaps you have Squares, Polygons, and Lines. Or TextInEnglish and TextInArabic. Or ... the point is, you probably have a relatively small number of concrete things and they probably all go in the same superordinate category. Similarly, you do not have Workers. On the assumption that what you have is Dogs, Cats, and Horses, then you probably also have an Exerciser and a Groomer and a Veterinarian.

Think about your concrete problem in concrete terms. Implement only the classes and only the relationships that you actually need.

zwol
  • 135,547
  • 38
  • 252
  • 361
  • Hi Zack, thank you for the insight. I used Objects to stay general just for this example on StackOverflow. But in my use case, I have many concrete classes with methods which have strong cohesion with their purpose in the framework. For example, I may have a Model class interface with derived classes NeuralNetworkModel, LogisticRegressionModel, etc. – trianta2 Feb 26 '14 at 21:41
  • 1
    You will get better advice from this site if you leave in *some* information about your goals in the large. Here's another thing to meditate on: [the XY problem](http://www.perlmonks.org/?node_id=542341). – zwol Feb 26 '14 at 22:31
  • I added a good deal of information regarding my goals in the large. – trianta2 Feb 27 '14 at 14:22
1

The point is that you're not accessing the specific functionality through the interfaces. The whole reason for using interfaces is that you want all Cars to be made, fixed and featured ... If you're not going to use them in that way, don't use interfaces (and inheritance) at all, but simply check at user input time which car was chosen and instantiate the correct specialized objects.

I've changed your code a bit so that only at "car making" time there will be an upward dynamic_cast. I would have to know all the things you want to do exactly to create interfaces I would be really happy with.

class ICar {
public:
    ICar(void) {}
    virtual ~ICar(void) {}
    virtual void specialFeatureMethod(ICarFeatures *specialFeatures);
    virtual void specialPartsMethod(ICarParts *specialParts);
    virtual void specialRepairMethod(ICarParts *specialParts);
};

class FordCar : public ICar {
public:
    FordCar(void) {}
    ~FordCar(void) {}
    void specialFeatureMethod(ICarFeatures *specialFeatures) {
    //Access the specialFeatures through the interface
    //Do your specific Ford stuff
    }
    void specialPartsMethod(ICarParts *specialParts) {
    //Access the specialParts through the interface
    //Do your specific Ford stuff
    }
    void specialRepairMethod(ICarParts *specialParts) {
    //Access the specialParts through the interface
    //Do your specific Ford stuff
    }
};

class ICarFeatures {
public:
    ICarFeatures(void) {}
    virtual ~ICarFeatures(void) {}
    virtual void addFeature(UserInput feature) = 0;
};

class FordCarFeatures : public ICarFeatures{
public:
    FordCarFeatures(void) {}
    ~FordCarFeatures(void) {}
    void addFeature(UserInput feature){

        //extract useful information out of feature, ie:
        std::string name = feature.name;
        int value = feature.value;
        _fordFeature->specialAddFeatureMethod(name, value);
    }

    FordFeatureImpl* _fordFeature;
};

class ICarParts {
public:
    ICarParts(void) {}
    virtual ~ICarParts(void) {}
    virtual void addPart(UserInput part) = 0;
};

class FordCarParts :public ICarParts{
public:
    FordCarParts(void) {}
    ~FordCarParts(void) {}
    void addPart(UserInput part) {

        //extract useful information out of part, ie:
        std::string name = part.name;
        std::string dimensions = part.dimensions;
        _fordParts->specialAddPartMethod(name, dimensions);
    }
    FordPartsImpl* _fordParts;
};

class ICarMaker {
public:
    ICarMaker(void) {}
    virtual ~ICarMaker(void) {}
    virtual ICar* makeCar(ICarFeatures* features, ICarParts* parts) = 0;
};

class FordCarMaker {
public:
    FordCarMaker(void) {}
    ~FordCarMaker(void) {}
    ICar* makeCar(ICarFeatures* features, ICarParts* parts){

        return customFordMakerFunction(features, parts);
    }

    ICar* customFordMakerFunction(ICarFeatures* features, ICarParts* parts) {

        FordCar* fordCar =  new FordCar;

        fordCar->specialFeatureMethod(features);
        fordCar->specialPartsMethod(parts);

        return dynamic_cast<ICar*>(fordCar);
    }
};


class ICarFixer {
public:
    ICarFixer(void) {}
    virtual ~ICarFixer(void) {}
    virtual void fixCar(ICar* car, ICarParts* parts) = 0;
};


class FordCarFixer {
public:
    FordCarFixer(void) {}
    ~FordCarFixer(void) {}
    void fixCar(ICar* car, ICarParts* parts) {

        customFordFixerFunction(car, parts);
    }   

    void customFordFixerFunction(ICar* fordCar, ICarParts *fordParts){
        fordCar->specialRepairMethod(fordParts);
    }
};
neeohw
  • 555
  • 4
  • 12
  • In your `FordCar::specialFeatureMethod` method, how does `FordCar` access the `FordFeatureImpl` object located within `FordCarFeatures` without dynamic casting, since `FordCar::specialFeatureMethod` only accepts an `ICarFeatures` object? – trianta2 Mar 05 '14 at 14:54
  • You will have to define functions on the ICarFeatures interface that manipulate the internals of the specific FordFeatures. These functions should be used to manipulate the BMWFeatures and BuickFeatures as well. Same goes for ICarParts. Currently I would remove the whole ICarFixer interface since it only calls one function on the ICar interface. But as I said, I would have to know more details to define better interfaces. – neeohw Mar 06 '14 at 08:16
0

One can do better (for certain values of "better"), with increased complexity.

What is actually being done here? Let's look point by point:

  • There's some object type, unknown statically, determined at run time from a string
  • There's some worker type, also unknown statically, determined at run time from another string
  • Hopefully the object type and the worker type will match

We can try to turn "hopefully" into "certainly" with some template code.

ObjectWorkerDispatcher* owd = 
    myDispatcherFactory->create("someWorker", "someObject");
owd->dispatch();

Obviously both object and worker are hidden in the dispatcher, which is completely generic:

class ObjectWorkerDispatcher {
   ObjectWorkerDispatcher(string objectType, string workerType) { ... }
   virtual void dispatch() = 0;
}

template <typename ObjectType>
class ConcreteObjectWorkerDispatcher : public ObjectWorkerDispatcher {
  void dispatch () {
    ObjectFactory<ObjectType>* of = findObjectFactory(objectTypeString);
    WorkerFactory<ObjectType>* wf = findWorkerFactory(workerTypeString);
    ObjectType* obj = of->create();
    Worker<ObjectType>* wrk = wf->create();
    wrk->doWork(obj);        
  }

   map<string, ObjectFactory<ObjectType>*> objectFactories;
   map<string, WorkerFactory<ObjectType>*> workerFactories;
   ObjectFactory<ObjectType>* findObjectFactory(string) { .. use map }
   WorkerFactory<ObjectType>* findWorkerFactory(string) { .. use map }
}

We have different unrelated types of Object. No common Object class, but we can have e.g. several subtypes of StringObject, all compatible with all kinds of StringWorker.

We have an abstract Worker<ObjectType> class template and concrete MyStringWorker : public Worker<StringObject> , OtherStringWorker : public Worker<StringObject> ... classes.

Both kinds of factories are inheritance-free. Different types of factories are kept completely separate (in different dispatchers) and never mix.

There's still some amount of blanks to fill in, but hopefully it all should be more or less clear.

No casts are used in making of this design. You decide whether this property alone is worth such an increase in complexity.

n. m. could be an AI
  • 112,515
  • 14
  • 128
  • 243
0

I think you have the right solution per your needs. One thing I see that can be improved is removing the use of carType from the function that deals with the objects at the base class level.

ICar* FordCarFixer::getFixedCar(UserInput& userInput)
{
   FordCarParts* carParts = new FordPartFactory;
   carParts->addPart(userInput);

   FordCarFeatures* carFeatures = new FordCarFeatures;
   carFeatures->addFeature(userInput);

   FordCarMaker* carMaker = new FordCarMaker;
   FordCar* car = carMaker->makeCar(carFeatures, carParts);

   UserInput repairSpecs = getUserInput();
   ForCarParts* replacementParts = new ForCarParts;
   replacementParts->addPart(repairSpecs);

   FordCarFixer* carFixer = new FordCarFixer;
   carFixer->fixCar(car, replacementParts);

   return car;
}

UserInput userInput = getUserInput();
ICar* car = CarFixerFactory::getFixedCar(userInput);

With this approach, most of the objects at FordCarFixer level are Ford-specific.

R Sahu
  • 204,454
  • 14
  • 159
  • 270