Logo Questions Linux Laravel Mysql Ubuntu Git Menu
 

Am I Abusing Inheritance Here? What's A Best-Practice Alternative/Pattern?

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.

like image 336
trianta2 Avatar asked Feb 26 '14 19:02

trianta2


3 Answers

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.

like image 56
Claudiordgz Avatar answered Oct 20 '22 18:10

Claudiordgz


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.

like image 26
zwol Avatar answered Oct 20 '22 19:10

zwol


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);
    }
};
like image 43
neeohw Avatar answered Oct 20 '22 20:10

neeohw