Is this a design flaw?

Question

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

1. class A is totally empty.
2. 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?

Answer 1

IMO AInternal makes no sense. Whatever you do there, should be done in AImpl. Otherwise, it's ok to do that in C++.

Answer 2

The code is rather obtuse, so I would be concerned with maintaining it six months down the road.

Answer 3

You're overcomplicating things by using 3 classes. I think what you're looking for is the pimpl idiom.

Answer 4

If you're going to do it this way, then the destructor ~A needs to be virtual.

Answer 5

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;

Answer 6

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.