Skip to main content
replaced http://codereview.stackexchange.com/ with https://codereview.stackexchange.com/
Source Link

From an MVC perspective, your controllers are doing far too much. Anything that's related processing on domain concepts (users, logins, products, etc) belongs in the model.

The entire point of MVC is Separation of Concerns so that each component is reusable/replaceable.

To test Separation of Concerns there are a few questions you can ask:

Are any of the components tightly coupled to the others?

In this case, both your models and views are very tightly coupled to your controllers. It's impossible for you to use a different view or model with that controller because the logic that chooses which view to use and which model to use exists within the controller itself. SoC would suggest separating these responsibilities.

If I replace a component will that involve repeating code?

If replacing a component requires rewriting lines of code this is a good indicator of poor Separation of Concerns. In your example, it's impossible to substitute the controller without repeating a lot of code. This limits flexibility and adds extra work. If you then update your view (which should be reusable) you then need to update every single controller that uses the view.

Controllers are not mediators between models and views. They take user input, update the model in some manner then the view requests relevant data from the model. See my answer here: Small MVC for PHPSmall MVC for PHP for a discussion about a very similar implementation.

For your implementation specifically logic in the form of "Display this if the user is logged in else display this" is by definition display logic. As such, it belongs in the view and not the controller.

The controller should not be telling the view to render. In fact, in a web MVC implementation, the controller doesn't even need to know of the view's existence (and in the implementations that do it's for a very specific reason). See the wikipedia diagram on MVC:

Model-view-controller

http://en.wikipedia.org/wiki/Model%E2%80%93view%E2%80%93controller

Which states: "A view requests from the model the information that it needs to generate an output representation."

From an MVC perspective, your controllers are doing far too much. Anything that's related processing on domain concepts (users, logins, products, etc) belongs in the model.

The entire point of MVC is Separation of Concerns so that each component is reusable/replaceable.

To test Separation of Concerns there are a few questions you can ask:

Are any of the components tightly coupled to the others?

In this case, both your models and views are very tightly coupled to your controllers. It's impossible for you to use a different view or model with that controller because the logic that chooses which view to use and which model to use exists within the controller itself. SoC would suggest separating these responsibilities.

If I replace a component will that involve repeating code?

If replacing a component requires rewriting lines of code this is a good indicator of poor Separation of Concerns. In your example, it's impossible to substitute the controller without repeating a lot of code. This limits flexibility and adds extra work. If you then update your view (which should be reusable) you then need to update every single controller that uses the view.

Controllers are not mediators between models and views. They take user input, update the model in some manner then the view requests relevant data from the model. See my answer here: Small MVC for PHP for a discussion about a very similar implementation.

For your implementation specifically logic in the form of "Display this if the user is logged in else display this" is by definition display logic. As such, it belongs in the view and not the controller.

The controller should not be telling the view to render. In fact, in a web MVC implementation, the controller doesn't even need to know of the view's existence (and in the implementations that do it's for a very specific reason). See the wikipedia diagram on MVC:

Model-view-controller

http://en.wikipedia.org/wiki/Model%E2%80%93view%E2%80%93controller

Which states: "A view requests from the model the information that it needs to generate an output representation."

From an MVC perspective, your controllers are doing far too much. Anything that's related processing on domain concepts (users, logins, products, etc) belongs in the model.

The entire point of MVC is Separation of Concerns so that each component is reusable/replaceable.

To test Separation of Concerns there are a few questions you can ask:

Are any of the components tightly coupled to the others?

In this case, both your models and views are very tightly coupled to your controllers. It's impossible for you to use a different view or model with that controller because the logic that chooses which view to use and which model to use exists within the controller itself. SoC would suggest separating these responsibilities.

If I replace a component will that involve repeating code?

If replacing a component requires rewriting lines of code this is a good indicator of poor Separation of Concerns. In your example, it's impossible to substitute the controller without repeating a lot of code. This limits flexibility and adds extra work. If you then update your view (which should be reusable) you then need to update every single controller that uses the view.

Controllers are not mediators between models and views. They take user input, update the model in some manner then the view requests relevant data from the model. See my answer here: Small MVC for PHP for a discussion about a very similar implementation.

For your implementation specifically logic in the form of "Display this if the user is logged in else display this" is by definition display logic. As such, it belongs in the view and not the controller.

The controller should not be telling the view to render. In fact, in a web MVC implementation, the controller doesn't even need to know of the view's existence (and in the implementations that do it's for a very specific reason). See the wikipedia diagram on MVC:

Model-view-controller

http://en.wikipedia.org/wiki/Model%E2%80%93view%E2%80%93controller

Which states: "A view requests from the model the information that it needs to generate an output representation."

added 597 characters in body
Source Link
Tom B
  • 276
  • 1
  • 4

From an MVC perspective, your controllers are doing far too much. Anything that's related processing on domain concepts (users, logins, products, etc) belongs in the model.

The entire point of MVC is Separation of Concerns so that each component is reusable/replaceable.

To test Separation of Concerns there are a few questions you can ask:

Are any of the components tightly coupled to the others?

In this case, both your models and views are very tightly coupled to your controllers. It's impossible for you to use a different view or model with that controller because the logic that chooses which view to use and which model to use exists within the controller itself. SoC would suggest separating these responsibilities.

If I replace a component will that involve repeating code?

If replacing a component requires rewriting lines of code this is a good indicator of poor Separation of Concerns. In your example, it's impossible to substitute the controller without repeating a lot of code. This limits flexibility and adds extra work. If you then update your view (which should be reusable) you then need to update every single controller that uses the view.

Controllers are not mediators between models and views. They take user input, update the model in some manner then the view requests relevant data from the model. See my answer here: Small MVC for PHP for a discussion about a very similar implementation.

For your implementation specifically logic in the form of "Display this if the user is logged in else display this" is by definition display logic. As such, it belongs in the view and not the controller.

The controller should not be telling the view to render. In fact, in a web MVC implementation, the controller doesn't even need to know of the view's existence (and in the implementations that do it's for a very specific reason). See the wikipedia diagram on MVC:

Model-view-controller

http://en.wikipedia.org/wiki/Model%E2%80%93view%E2%80%93controller

Which states: "A view requests from the model the information that it needs to generate an output representation."

From an MVC perspective, your controllers are doing far too much. Anything that's related processing on domain concepts (users, logins, products, etc) belongs in the model.

Controllers are not mediators between models and views. They take user input, update the model in some manner then the view requests relevant data from the model. See my answer here: Small MVC for PHP for a discussion about a very similar implementation.

For your implementation specifically logic in the form of "Display this if the user is logged in else display this" is by definition display logic. As such, it belongs in the view and not the controller.

The controller should not be telling the view to render. In fact, in a web MVC implementation, the controller doesn't even need to know of the view's existence (and in the implementations that do it's for a very specific reason). See the wikipedia diagram on MVC:

Model-view-controller

http://en.wikipedia.org/wiki/Model%E2%80%93view%E2%80%93controller

Which states: "A view requests from the model the information that it needs to generate an output representation."

From an MVC perspective, your controllers are doing far too much. Anything that's related processing on domain concepts (users, logins, products, etc) belongs in the model.

The entire point of MVC is Separation of Concerns so that each component is reusable/replaceable.

To test Separation of Concerns there are a few questions you can ask:

Are any of the components tightly coupled to the others?

In this case, both your models and views are very tightly coupled to your controllers. It's impossible for you to use a different view or model with that controller because the logic that chooses which view to use and which model to use exists within the controller itself. SoC would suggest separating these responsibilities.

If I replace a component will that involve repeating code?

If replacing a component requires rewriting lines of code this is a good indicator of poor Separation of Concerns. In your example, it's impossible to substitute the controller without repeating a lot of code. This limits flexibility and adds extra work. If you then update your view (which should be reusable) you then need to update every single controller that uses the view.

Controllers are not mediators between models and views. They take user input, update the model in some manner then the view requests relevant data from the model. See my answer here: Small MVC for PHP for a discussion about a very similar implementation.

For your implementation specifically logic in the form of "Display this if the user is logged in else display this" is by definition display logic. As such, it belongs in the view and not the controller.

The controller should not be telling the view to render. In fact, in a web MVC implementation, the controller doesn't even need to know of the view's existence (and in the implementations that do it's for a very specific reason). See the wikipedia diagram on MVC:

Model-view-controller

http://en.wikipedia.org/wiki/Model%E2%80%93view%E2%80%93controller

Which states: "A view requests from the model the information that it needs to generate an output representation."

added 9 characters in body
Source Link
Tom B
  • 276
  • 1
  • 4

From an MVC perspective, your controllers are doing far too much. Anything that's related processing on domain concepts (users, logins, products, etc) belongs in the model.

Controllers are not mediators between models and views. They take user input, update the model in some manner then the view requests relevant data from the model. See my answer here: Small MVC for PHP for a discussion about a very similar implementation.

For your implementation specifically logic in the form of "Display this if the user is logged in else display this" is by definition display logic. As such, it belongs in the view and not the controller.

The controller should not be telling the view to render. In fact, in mosta web MVC implementationsimplementation, the controller doesn't even need to know of the view's existence (and in the implementations that do it's for a very specific reason). See the wikipedia diagram on MVC:

Model-view-controller

http://en.wikipedia.org/wiki/Model%E2%80%93view%E2%80%93controller

Which states: "A view requests from the model the information that it needs to generate an output representation."

From an MVC perspective, your controllers are doing far too much. Anything that's related processing on domain concepts (users, logins, products, etc) belongs in the model.

Controllers are not mediators between models and views. They take user input, update the model in some manner then the view requests relevant data from the model. See my answer here: Small MVC for PHP for a discussion about a very similar implementation.

For your implementation specifically logic in the form of "Display this if the user is logged in else display this" is by definition display logic. As such, it belongs in the view and not the controller.

The controller should not be telling the view to render. In fact, in most MVC implementations, the controller doesn't even know of the view's existence (and in the implementations that do it's for a very specific reason). See the wikipedia diagram on MVC:

Model-view-controller

http://en.wikipedia.org/wiki/Model%E2%80%93view%E2%80%93controller

Which states: "A view requests from the model the information that it needs to generate an output representation."

From an MVC perspective, your controllers are doing far too much. Anything that's related processing on domain concepts (users, logins, products, etc) belongs in the model.

Controllers are not mediators between models and views. They take user input, update the model in some manner then the view requests relevant data from the model. See my answer here: Small MVC for PHP for a discussion about a very similar implementation.

For your implementation specifically logic in the form of "Display this if the user is logged in else display this" is by definition display logic. As such, it belongs in the view and not the controller.

The controller should not be telling the view to render. In fact, in a web MVC implementation, the controller doesn't even need to know of the view's existence (and in the implementations that do it's for a very specific reason). See the wikipedia diagram on MVC:

Model-view-controller

http://en.wikipedia.org/wiki/Model%E2%80%93view%E2%80%93controller

Which states: "A view requests from the model the information that it needs to generate an output representation."

Source Link
Tom B
  • 276
  • 1
  • 4
Loading