Consider two classes class A{ public: A(){ } ~A(){ } }; class AImpl : public A{ public: AImpl(){ a = new AInternal(); } AImpl(AInternal *a){ this->_a = a; } ~AImpl(){ if(a){ delete a; a = null; } } private: AInternal *a; }; I am trying to hide the AInternal's implementation and expose only A's interface. Two things I see here class A is totally empty. Hiding is achieved basically through inheritance. I have to actually use downcasting and upcasting from A to AImpl and vice versa. Is this a good design. Being very inexperienced in designing, I cannot see the pitfalls of it and why it is bad?

Reputation: 7864

Is this a design flaw?

Consider two classes

class A{
     public:
       A(){
       }
       ~A(){
       }
};


class AImpl : public A{
      public:
         AImpl(){
             a = new AInternal();
         }
         AImpl(AInternal *a){
             this->_a = a;
         }
         ~AImpl(){
             if(a){
                delete a;
                a = null;
             }
         }
       private:
             AInternal *a;
};

I am trying to hide the AInternal's implementation and expose only A's interface. Two things I see here

class A is totally empty.
Hiding is achieved basically through inheritance. I have to actually use downcasting and upcasting from A to AImpl and vice versa.

Is this a good design. Being very inexperienced in designing, I cannot see the pitfalls of it and why it is bad?

Upvotes: 0

Answers (6)

qba

Reputation: 1311

I am trying to hide the AInternal's implementation and expose only A's interface.

I think you are trying to do something like factory.

Here is an example:

 class IA {                                               
 public:
         IA() {}
         virtual ~IA() {}
         virtual void dosth() =0;
 };

 class Factory {
 private:
         class A : public IA {
         public:
                 A () {}
                 virtual ~A() {}

                 void dosth() { cout << "Hello World"; }
         };

 public:
         Factory () {}
         virtual ~Factory() {}

         IA*newA() { return new A; }
 };

And the usage of Factory class:

Factory f;
IA*a = f.newA();
a->dosth();
return 0;

Upvotes: 1

Steve Jessop

Reputation: 279315

You seem to be combining two common design features:

1) AInternal is a "pimpl". It provides for better encapsulation, for example if you need to add a new field to AInternal, then the size of AImpl doesn't change. That's fine.

2) A is an base class used to indicate an interface. Since you talk about upcasting and downcasting, I assume you want dynamic polymorphism, meaning that you'll have functions which pass around pointers or references to A, and at runtime the referands will actually be of type AImpl. That's also fine, except that A's destructor should either be virtual and public, or non-virtual and protected.

I see no other design problems with this code. Of course you'll need to actually define the interface A, by adding some pure virtual member functions to it that you implemented in AImpl. Assuming you plan to do that, there's nothing wrong with using an empty base class for the purpose which in Java is served by interfaces (if you know Java). Generally you'd have some kind of factory which creates AImpl objects, and returns them by pointer or reference to A (hence, upcasts them). If the client code is going to create AImpl objects directly then that might also be fine, and in fact you might not need dynamic polymorphism at all. You could instead get into templates.

What I don't see is why you would ever have to downcast (that is, cast an A* to AImpl*). That's usually bad news. So there may be some problems in your design which can only be revealed by showing us more of the definitions of the classes, and the client code which actually uses A and AImpl.

Upvotes: 0