3
\$\begingroup\$

I am kind of new to MVC, I've read lots of tutorials online but each keep using different approaches so I decided to create mine. Below is what my code looks like.

index

$url_segments = explode('/', $url);
$controller = !empty($url_segments[0]) ? $url_segments[0] : 'home';
array_shift($url_segments); // removes the controller name from array
$action = isset($url_segments[0]) && !empty($url_segments[0]) ? $url_segments[0] : 'index';
array_shift($url_segments); // removes the action name from array

$controller = ucfirst($controller);
$controller = new $controller($url_segments);
$controller->$action();

controller

class Controller
{
   protected $model;
   protected $view;

   public function __construct() {
        $this->view = new View();
   }
}

model

class Model 
{
    private $connection;

    public function __construct() {
        require_once DOCUMENT_ROOT . '/resources/connection.php';

        $db = new DatabaseFactory();
        $this->connection = $db->getFactory()->getConnection('localhost', 'db_un', 'db_pw', 'db_nm');
    }

    public function getConnection() {
        return $this->connection;
    }
}

view

class View
{
    private $file;
    private $data;

    public function output($file, $data = null, $content_only = false) {
        if (isset($data) && is_array($data)) {
            foreach ($data as $key=>$value) {
                ${$key} = $data[$key];
            }
        }

        ($content_only == false) && require_once TEMPLATES_PATH . '/header.php';
        require_once $file;
        ($content_only == false) && require_once TEMPLATES_PATH . '/footer.php';
    }
} 

For example if the url example/accounts/add/student was visited,

  • controller would be 'accounts'
  • action would be 'add'
  • parameter ($url_segment[0]) would be 'student'

Accounts class

class Accounts {
    public function __construct() {
        parent::__construct();
    }

    public function add() {
        if (!isset($this->url_segments[0])) { 
            // url entered was: example.com/add so query type

            // rendering view
            $this->view->output(VIEW . 'query_account_type_to_add.php');
        } else if (isset($this->url_segments[0]) && $this->url_segments[0] == 'student') {
            // code to add student account

            // rendering view
            $this->view->output(VIEW . 'add_account.php');
        }
    }
}

Is this really MVC?

Secondly, my controller uses conditional statement based on parameters supplied after the controller and action part of the url to call different views, and I've read somewhere that having controller decide what view to load is bad and MVC would not have been structured properly. Are there any changes I'll need to be making to my code above?

\$\endgroup\$
2
\$\begingroup\$

my controller uses conditional statement based on parameters supplied after the controller and action part of the url to call different views, and I've read somewhere that having controller decide what view to load is bad and MVC

I wouldn't worry about that. The definition of MVC has changed over the years, and the approach(es) that are generally taken in web applications are quite different from the original approach.

All major frameworks that I am familiar with sometimes have a controller decide what view to show (the add/edit cases are classic examples).

Router

Your index file is your router, but it is very primitive.

Specifically, you will have trouble changing how your URLs look, and you have no input validation.

Right now, a user can create arbitrary objects (whose constructor accepts an array), and then call arbitrary methods on it (without arguments though). While difficult to exploit, this is certainly a security concern.

For an idea of how a more rounded router might work, see here (I'm linking to my own question because I could easily find it; the code is certainly not ideal, but it shows the basic idea; you want to have a central point where you define what route matches which controller methods, and you want to only allow those to be called; you even get primitive input validation (eg string or integer) of arguments for free).

Controller

I would pass the arguments to the method, not the constructor. Currently, you are working on a magic array where you have no idea what it may contain. This makes it very difficult to properly operate on it. Debugging, validating the data, documenting the code, and reading the code all get more difficult than necessary.

So it might be:

public function add($studentName, ...) [...]
public function delete($id) [...]
public function show($id) [...]
[...]

Now you know exactly what arguments the methods take. You will need to slightly edit your router though.

View

I would consider using a templating engine. It results in nicer code, and has security benefits as well (automatic encoding against XSS, etc). You should then be able to get rid of your View class completely.

Model

This is the main point I would criticize regarding MVC. This isn't a model at all, it is a class holding the database connection.

A model would for example be:

class Student {
    private $id;
    private $name;

    [constructor, getter, setter]
    [operations; I can't think of reasonable ones for students right now, but something like "teach($topic)"]
}

You could now use this student class in your controller to perform operations, get info, change info, etc. You can also use it in your views.

This way, you decouple your controller and view. The student model now defines how a student looks and what they can do.

You might now also want to add a student DAO, which retrieves students from the database, updates them, etc.

Misc

  • I would add Controller to the controller class name, it makes it easier to understand. Same for model and view.
  • add is an uncommon name (add to what?). I would go with create.
\$\endgroup\$
  • \$\begingroup\$ Thank you very much @tim, I'm finally studying codeigniter's MVC design. One interesting thing you said is passing argurments to the controller's methods, consider the url /questions/show/mcq/24 where '24' is the course id and another /questions/show/mcq/24/300 where the '24' is the course id and '300' is the question id, if the Questions::show() is called, 3 potential argurments to be passed are {type (mcq or essay)} ,{course_id}, {question_id} (which may or may not be present), so the method show() will not know how many argurments to expect, should i use overloaded methods? \$\endgroup\$ – carl frank Aug 29 '17 at 4:06
  • \$\begingroup\$ @carlfrank Good question. That would definitely be a possibility. It's not quite clear to me how courses and questions relate to each other (it's a bit odd that /questions has a mantatory course id but an optional questions id), so I can only guess here. \$\endgroup\$ – tim Aug 29 '17 at 8:46
  • 1
    \$\begingroup\$ @carlfrank I think you could probably avoid this problem in the first place by designing these methods more clearly. Eg you might have questions/index (listing all questions), courses/index (listing all courses), questions/show/[id] (show question with id "id"), questions/index/course/[id] (list all questions for given course id) (or course/[id]/questions if it makes more sense in your context, which is probably true). If required you can also have question/[id]/course/[id] although I don't think it's necessary. Either way, this is a rather large topic to go into in a comment^^ \$\endgroup\$ – tim Aug 29 '17 at 8:47
2
\$\begingroup\$

You are on the right path. Your implementation is MVC.

The only thing your design is lacking is inheritance.

<?php
/**
* Model
*/
class AccountsModel extends Model {

}

/**
* Controller
*/
class AccountsController extends Contoller {

public function index(){
    $accountsModel = new AccountsModel();
    $accountsData = $accountsModel->getLatest(100); // get latest 100 accounts
    $this->view->accountsData = $accountsData;
 }
}

/**
 * view file
 * /accounts/index
 */
foreach($accountsData as $data){
  // do something with $data
}

To answer your second question. The business logic should be done in the controller. In an ideal world you would only echo stuff in the view, and no logic should be done in the view.

I would suggest looking at CakePHP3's MVC / Symphony MVC. It is very well done and it would make things clearer.

\$\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.