Skip to main content
Code Review

Return to Answer

replaced http://stackoverflow.com/ with https://stackoverflow.com/
Source Link

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 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? 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.

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.

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.

Source Link
Loki Astari
  • 97.7k
  • 5
  • 126
  • 341

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.

lang-cpp

AltStyle によって変換されたページ (->オリジナル) /