Is this a good code (came across while reading code of a colleague)

Question

File a.hpp:

class a;
typedef boost::shared_ptr<a> aPtr

class a{
public:
    static aPtr CreateImp();
    virtual void Foo() = 0 ;
    ....
};

File aImp.hpp:

class aImp : public a{

    virtual void Foo();

};

File aImp.cpp:

aPtr a::CreateImp()
{
     return aPtr(new aImp());
}

void aImp::Foo(){}

The client must use CreateImp to get pointer to a, and can't use a other ways. What do you think about this implementation? What do you think about this kind of implementation?

Answer 1

This looks like a normal implementation if the Factory Method design pattern. The return of boost::shared_ptr just makes life of the programmer using this API easier in terms of memory management and exception safety and guards against simple mistakes like calling the function and ignoring the return value.

Edit:

If this is the only implementation of the base class, then it might be that the author was aiming for pimpl idiom to hide the implementation details and/or reduce compile-time dependencies.

Answer 2

Looks like good encapsulation, although I don't actually see anything preventing a from being used otherwise. (For example, private constructor and friend class aImp).

No compile unit using class a will have any knowledge of the implementation details except for aImp.cpp itself. So changes to aImp.hpp won't induce recompiles across the project. This both speeds recompilation and prevents coupling, it's helps maintainability a lot.

OTOH, anything implemented in aImp.hpp now can't be inlined, so run-time performance may suffer some, unless your compiler has something akin to Visual C++ "Link-Time Code Generation" (which pretty much undoes any gain to build speed from encapsulation).

As far as the smart pointer is concerned, this depends on project coding standards (whether boost pointers are used everywhere, etc).

Answer 3

If the intention is using the PIMPL idiom, then it is not the most idiomatic way. Inheritance is the second strongest coupling relationship in C++ and should be avoided if other solutions are available (i.e. composition).

Now, there might be other requirements that force the use of dynamic allocation, and/or the use of an specific type of smart pointer, but with the information you have presented I would implement the PIMPL idiom in the common way:

// .h
class a {
public:
   ~a(); // implement it in .cpp where a::impl is defined
   void op();   
private:
   class impl;
   std::auto_ptr<impl> pimpl;
};
// .cpp
class a::impl {
public:
   void op();
};
a::~a() {} 
void a::op() { pimpl->op(); }

The only (dis)advantage of using inheritance is that the runtime dispatch mechanism will call the implementation method for you, and you will not be required to implement the forwarding calls (a::op() in the example). On the other hand, you are paying the cost of the virtual dispatch mechanism in each and every operation, and limiting the use of your class to the heap (you cannot create an instance of a in the stack, you must call the factory function to create the object dynamically).

On the use of shared_ptr in the interface, I would try to avoid it (leave freedom of choice to your users) if possible. In this particular case, it seems as if the object is not really shared (the creator function creates an instance and returns a pointer, forgetting about it), so it would be better to have a smart pointer that allows for transfer of ownership (either std::auto_ptr, or the newer unique_ptr could do the trick), since the use of shared_ptr imposes that decision to your users.

(NOTE: removed it as the comment makes it useless)