3
\$\begingroup\$

I am creating a controller that will display a form and once the form has been submitted to store the item in the database. After the item has been stored the user will be redirected back to the main page of the site. I am calling this controller with:

require_once("../app/controller/AddController.php");
new AddController();

The AddController.php file looks like this:

<?php

class AddController {

    public function __construct(){

        //Display our form
        if (empty($_POST['category']) || empty($_POST['description'])){

            require_once("../app/view/AddView.php");
            new AddView(array('title' => "Add an Item"));

        }

        //Add our new item to the database
        else {

            require_once("../app/model/AddModel.php");
            $add = new AddModel($_POST);

            if ($add->success())
                $_SESSION['message']['success'] = "The item has been added.";
            else
                $_SESSION['message']['error'] = "There was a problem adding your item.";

            header("Location: /");
            exit;

        }
    }
}

My main question is whether it is bad practice to place everything in the __construct() method like this since it is not really instantiating any variables or if I should get rid of __construct() and create a different method. The main reason I was doing this was to eliminate the third line when calling this class:

require_once("../app/controller/AddController.php");
$add = new AddController();
$add->someFunction();
\$\endgroup\$
1
\$\begingroup\$

There isn't a huge difference between putting all code in the constructor, or putting all code in a method that must always be called after construction. And both approaches aren't all that great, as having one giant method is hard to maintain, hard to read, hard to test, and hard to use.

But first things first, what is an AddController? Generally, controllers are associated with some kind of item, not some kind of action. So you might have a SpecificItemController, which can add, edit, update, delete, show, list, etc SpecificItems. But you wouldn't have an AddController which can add SpecificItemA, SpecificItemB, etc. (you might have controllers that aren't directly item specific, but more action specific, such as a LoginController, but that's a bit different).

In this controller, you would now have methods that are responsible for the actions that can be taken. So you might have a show method, an add method, etc. Then you would create a router, which would link a given URL to the correct method in the correct controller.

Misc

  • You only asked about the controller, but your model apparently also does most of its work in the constructor. Having a functionality such as inserting something into a database in the constructor is not a good idea. It also shows that you didn't really apply MVC. An AddModel doesn't make sense. You want an ItemModel, which stores the item data, and which may also be responsible for the business logic, and possibly for saving and loading things in the db (or move that to specific db access objects).
  • Same goes for the AddView. An ItemView would make more sense.
  • with proper methods, your comments are not needed anymore. Remove them and instead add proper PHPDoc comments documenting the method.
  • always use curly brackets to increase readability and avoid future bugs.
\$\endgroup\$
  • \$\begingroup\$ Okay, what about an object that displays all items. Would that also be a part of the ItemController/Model/View or because it's not dealing with a specific item would I create a separate ItemsController/Model/View? \$\endgroup\$ – kojow7 Apr 20 '16 at 18:37
  • \$\begingroup\$ @kojow7 item is rather vague. I tried to keep your terminology (except for SpecificItem, but you can read it as Item), but it's not ideal, and I'm not quite sure what you actually mean by it. For clarity, let's say the item in question is a category. Then that CategoryView could have a list method, which lists an array of categories. But it shouldn't eg list Users. You could have a BaseView, which is extended by other items, and which contains some method that can be used to show generic items, but it's likely not applicable in many situations. \$\endgroup\$ – tim Apr 20 '16 at 18:53
1
\$\begingroup\$

If we talk about good practice then "Better not to do much in __constructor()". Constructors are built to instantiate a class. They must not be overloaded with huge code. For providing variant functionalities, use methods instead. This is how to work in modules. For a specific kind of task, it is better to call the specific function instead of overloading the same method.

However, you can do so (and you are doing), but it is not a good approach. Consider an example,

You have to perform 10 actions for a given event. You can do so in same function. But it would one day become much complex to add new functionalities in future. However if you are dividing the whole task in modules, you can and will be able to add new functionalities with no overhead. Also this makes your code:

  1. General
  2. Flexible
  3. Easily understandable
  4. Easily modifiable
  5. Clean
  6. Best practice

Also not all frameworks follow complete MVC but you can see in any one of them, separate functions and classes are there for specific task/action. This may cause to write more lines of code but since this is a good practice, almost all the developers follow the same mechanism.

However, it is user specific opinion. At last, you can do anything you want to do cos you are developing & may be this approach is somehow good in your context. Choice is yours.

\$\endgroup\$

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service, privacy policy and cookie policy

Not the answer you're looking for? Browse other questions tagged or ask your own question.