Implementation of inheritable pImpl idiom and MVC pattern

Question

I am trying to write an object "template" that use pImpl and MVC pattern to encapsulate everything not specific to the object. The following is how I tried to attempt it. I am sure that the code smells (if not stinks) but can't tell what's wrong with the design.

I would like to know what better alternatives with which I can do this.

#include <iostream>
using namespace std;
class Ancestor{}; //every object using the "template" inherits this class
//use a template as the type of IMPL is only known at compile time
template <typename IMPL> class Obj : public Ancestor {
 public:
 //dependency injection for easy TDD
 Obj(IMPL *impl = new IMPL()) : m_pImpl(impl) {};
 ~Obj() {delete m_pImpl; m_pImpl = nullptr;};
 protected:
 IMPL *m_pImpl;
};
// well a template is used for the same reason as for Obj<>
template <typename MODEL> struct MVCController {
 MODEL *m_pModel;
};
// same reason for a template
template <typename MODEL, typename CONTROLLER> struct MVCImpl {
 MODEL *m_pModel;
 CONTROLLER *m_pController;
 // leave alone the View for simplicity
 //constructor
 MVCImpl(MODEL *m = new MODEL(), CONTROLLER *c = new CONTROLLER()) :
 m_pModel(m), m_pController(c) {m_pController->m_pModel = m;};
};
struct someBaseModel{};

If I want to create an object called myObj with a public API void foo():

struct myModel : someBaseModel {
 // POD
};
struct myController : public MVCController<myModel> {
 void foo() {cout << "myController: foo" << endl;};
};
struct myImpl : public MVCImpl<myModel, myController> {};
class myObj : public Obj<myImpl> {
 public:
 // inherit myBaseObj to call its default ctor/dtor if no customization is needed
 myObj(myImpl *impl = new myImpl()) : Obj<myImpl>(impl) {};
 void foo();
};
void myObj::foo() {m_pImpl->m_pController->foo();};

If I want myDerivedObj to inherit myObj and add a void bar():

struct myDerivedController : public myController {
 void bar() {cout<<"myDerivedController: bar"<<endl;};
};
struct myDerivedImpl : public MVCImpl<myModel, myDerivedController> {};
// virtual inheritance is used as both inherited class inherits from Ancestor
class myDerivedObj : virtual public myObj, virtual public Obj<myDerivedImpl> {
 using Obj<myDerivedImpl>::m_pImpl;
 public:
 myDerivedObj(myDerivedImpl *impl = new myDerivedImpl()) :
 Obj<myDerivedImpl>(impl) {};
 // add any new API here
 void bar();
};
void myDerivedObj::bar() {m_pImpl->m_pController->bar();};
int main() {
 auto m2 = new myDerivedObj();
 m2->foo();
 m2->bar();
 return 0;
}

Output:

myController: foo
myDerivedController: bar

By using the above implementation I am trying to

1. hide all pImpl and MVC specific code in the templates
2. hide all generic "object behaviors" in Ancestor
3. leave the possibility for inheritance

• hailstone: The included code snippet are reordered to a working one.
• 200_success: This was closed not because the code was broken (maybe it was, maybe it wasn't), but because it's hypothetical code. Code with completely generic identifiers like myDerivedObj, foo, and bar aren't suitable for review.
• hailstone: I don't think it's hypothetical, or at least I don't know how to make it not so hypothetical. The code is supposed to be a generic "template" for all objects. So why bother with the exact object? Please let me know if I get something wrong here. Thanks.
• hailstone: To be exact, I am writing the above templates for a code generator in a visual game development engine. Hope this makes more sense.
• Loki Astari: Reopen: Templatized code is not theoretical ;-)

Answer 1

Stop doing this:

using namespace std;

Its a bad habit that you need to stop. Prefix your stuff with std:: and don't be lazy. The whole reason its std and not standard_lib is so that it would not be an issue using the prefix everywhere. Anyway there are issues that will crop up in real code see Why is "using namespace std;" considered bad practice?

Why.

class Ancestor{}; //every object using the "template" inherits this class

Just saying they do is not a good comment.
A good comment would explain why they need to. Also since there is no interface you are implementing I don't see a reason for this (maybe a comment would clarify).

In this class:

template <typename IMPL> class Obj : public Ancestor {

You are doing resource management. But you don't implement the rule of three (as such it is broken). Please make sure you correctly do resource management. What is The Rule of Three?

While we are here. You may also want to look at move semantics (its 2014 so you should be using at least C++11 or C++14). This will make your code more efficient in the 3% of places that RVO and NRVO don't apply. Also containers are now move aware that makes them more efficient.

Pointers in your class. Not good.

// well a template is used for the same reason as for Obj<>
template <typename MODEL> struct MVCController {
 MODEL *m_pModel;
};

A pointer does not semantically help with determining ownership. I assume a controller is always linked to a model (ie it is never null) so I think a reference would be a better way to represent the connection to the model. If you can potentially have a NULL model (I would not go this way) then maybe a std::shared_ptr as many controllers can potentially use the same model.

A lot of implementations combine the controller and model so that is fine.

// same reason for a template
template <typename MODEL, typename CONTROLLER> struct MVCImpl {
 MODEL *m_pModel;
 CONTROLLER *m_pController;
 // leave alone the View for simplicity
 //constructor
 MVCImpl(MODEL *m = new MODEL(), CONTROLLER *c = new CONTROLLER()) :
 m_pModel(m), m_pController(c) {m_pController->m_pModel = m;};
};

But you have an bad obsession about creating these things dynamically. Please use to learn how to use automatic objects. This makes resource management much easier.

Also you do need to figure out how the view(s) are connected to your MC object.

• hailstone: Intention of using pointer to model is for dependency injection for easy TDD. But obviously that's bad decision as copy constructor and a reference should be the right thing to do.
• hailstone: First time I hear Rule Of Three. Nice. namespace std - generated code as I am using a online compiler for the about code haha, good that you spot it. bad obsession about creating things dynamically - can you give an example or places I should look at? Like perhaps in the snippet you quoted change the arguments to auto type in MVCImpl constructor? View would be linked the same way as Controller. Thanks.