Logo Questions Linux Laravel Mysql Ubuntu Git Menu
 

Please critique my first attempt at MVC in PHP [closed]

Well I'm not a huge framework guy but I've been liking what I've been hearing about the whole MVC movement so I thought I'd try to create a simple application in my language of choice (PHP)

So I guess the question is: where did I go wrong? I know there is a lot of debate as to how fat the controller/model should be so hopefully we can avoid that, however I am especially curious as to your thoughts on how I fit the datatier in.

Also I bought a domain to do some testing so if you want to see it in action you can go to www.omgmvc.com

Firstly, here's my database schema:

CREATE TABLE `movies` (
  `id` int(11) NOT NULL auto_increment,
  `movie_name` varchar(255) NOT NULL,
  `release_date` date NOT NULL,
  `directors_name` varchar(255) NOT NULL,
  PRIMARY KEY  (`id`)
);

INSERT INTO `movies` VALUES (1,'Star Wars', '1977-05-25', 'George Lucas');
INSERT INTO `movies` VALUES (2,'The Godfather', '1972-03-24', 'Francis Ford Coppola');
INSERT INTO `movies` VALUES (3,'The Dark Knight', '2008-07-18', 'Christopher Nolan');

And here's the files:

index.php (controller)

<?php

include('datatier.php');
include('models/m_movie.php');

if (isset($_GET['movie']) && is_numeric($_GET['movie']))
{
    $movie = new Movie($_GET['movie']);

    if ($movie->id > 0)
    {
        include('views/v_movie.php');
    }
    else
    {
        echo 'Movie Not Found';
    }
}
else
{
    $movies = Movie::get_all();

    include('views/v_list.php');
}

?>

datatier.php (data tier)

<?php

class DataTier
{
    private $database;

    function __construct()
    {
        $this->connect();
    }

    function __destruct()
    {
        $this->disconnect();
    }

    function connect()
    {
        $this->database = new PDO('mysql:host=localhost;dbname=dbname','username','password');
    }

    function disconnect()
    {
        $this->database = null;
    }

    function get_all_from_database($type)
    {
        $database = new PDO('mysql:host=localhost;dbname=dbname','username','password');

        switch ($type)
        {
            case 'movie':
                $query = 'SELECT id FROM movies';
                break;
        }

        $movies = array();

        foreach ($database->query($query) as $results)
        {
            $movies[sizeof($movies)] = new Movie($results['id']);
        }

        $database = null;

        return $movies;
    }

    function get_from_database($type,$id)
    {
        switch ($type)
        {
            case 'movie':
                $query = 'SELECT movie_name,release_date,directors_name FROM movies WHERE id=?';
                break;
        }

        $database_call = $this->database->prepare($query);
        $database_call->execute(array($id));

        if ($database_call->rowCount() > 0)
        {
            return $database_call->fetch();
        }
        else
        {
            return array();
        }
    }
}

?>

models/m_movie.php (model)

<?php

class Movie extends DataTier
{
    public $id;
    public $movie_name;
    public $release_date;
    public $directors_name;

    function __construct($id)
    {
        parent::connect();

        $results = parent::get_from_database('movie',$id);

        if ($results == array())
        {
            $this->id = 0;
        }
        else
        {
            $this->id = $id;
            $this->movie_name = $results['movie_name'];
            $this->release_date = $results['release_date'];
            $this->directors_name = $results['directors_name'];
        }
    }

    function __destruct()
    {
        parent::disconnect();
    }

    static function get_all()
    {
        $results = parent::get_all_from_database('movie');

        return $results;
    }
}

?>

views/v_list.php (view)

<html>
    <head>
        <title>Movie List</title>
    </head>
    <body>
        <table border="1" cellpadding="5" cellspacing="5">
            <thead>
                <tr>
                    <th>Movie Name</th>
                    <th>Directors Name</th>
                    <th>Release Date</th>
                </tr>
            </thead>
            <tbody>
<?php foreach ($movies as $movie) { ?>
                <tr>
                    <td><a href="/?movie=<?php echo $movie->id; ?>"><?php echo $movie->movie_name; ?></a></td>
                    <td><?php echo $movie->directors_name; ?></td>
                    <td><?php echo $movie->release_date; ?></td>
                </tr>
<?php } ?>
            </tbody>
        </table>
    </body>
</html>

views/v_movie.php (view)

<html>
    <head>
        <title><?php echo $movie->movie_name; ?></title>
    </head>
    <body>
        <h1><?php echo $movie->movie_name; ?></h1>
        <h2>Directed by <?php echo $movie->directors_name; ?></h2>
        <h3>Released <?php echo $movie->release_date; ?></h3>
    </body>
</html>
like image 674
Andrew G. Johnson Avatar asked Jan 15 '09 12:01

Andrew G. Johnson


People also ask

Why do we use MVC in PHP?

MVC stands for "Model view And Controller". The main aim of MVC Architecture is to separate the Business logic & Application data from the USER interface.

Why is MVC so popular?

ADVANTAGES OF MVC Apart from isolating the view from the business logic, the M-V-C framework reduces complexity for developers modeling an information system, especially when designing large applications. The code is much more structured and therefore easier to maintain, test and reuse.


2 Answers

You're doing good, but i have a few suggestions:

  1. Since you are using php5 dont forget about __autoload function.
  2. It is better to name your data tier as Model.
  3. get_all_from_database is not declared as static but you are calling it statically, this generates a E_STRICT level warning. Set error_reporting(E_ALL | E_STRICT); and you should see the warning.
  4. get_all() static function should be in the Model class (in your case Datatier), so that you dont have to rewrite it for every other model. The only change you need to make in that function is to replace the line:

    $results = parent::get_all_from_database('movie');

with

$results = $this->get_all_from_database(get_class($this));

this assumes that the name of your models must match the names of your tables in the database

like image 24
Joey Avatar answered Oct 25 '22 06:10

Joey


First, you are doing pretty well on keeping things separated. It pays back in the future, so don't give up.

The database layout (or even database itself) is irrelevant to the essence of MVC. It happens to be relational database in most cases, however MVC does not require it explicitly (you can use XML storage or some grid/cloud as well). What is crucial to the MVC is to keep Model separated from the rest, which you did.

Your View is also clearly separated from the rest. Similarly to M part of MVC, Views can not only present HTML, but any text-representable output (XML, XML+XSL, RSS, plain text, or even email messages), Views can be implemented in several ways: PHP includes like yours, templates (i.e. Smarty) or fully fledged objects serializable to text. I'm far from judging which strategy is the best, it's a matter of individual coding style and project requirements.

Your controller is confusing (it is more Page Controller than Application Controller). It is probably due to the fact, that there's one hidden part in MVC architecture. It is called Front Controller or Dispatcher. It is up to Dispatcher to parse input, instantiate controller (as in Application Controller) and invoke requested method. If you want to keep going with your custom MVC implementation I suggest you using some common way of passing Controller class and method name in URL, i.e.

index.php/Movies/list
index.php/Movies/details/35

Then in new index.php you just parse $_SERVER['PATH_INFO'], instantiate class Movies and call its list method, i.e.

$args = explode('/', ltrim($_SERVER['PATH_INFO'], '/'));
$className = array_shift($args);
$method = array_shift($args);
require "$className.php";
call_user_func_array(array(new $className(), $method), $args);

Then you just move the content of if-else block to two separate methods in Movies class.

class Movies { // may extend generic Controller class if you wish

    public function list() {
        $movies = Movie::get_all();
        include 'views/v_list.php';
    }

    public function details($movieId) {
        $movie = new Movie($movieId);
        if ($movie->id > 0) {
            include 'views/v_movie.php';
        } else {
            echo "Movie Not Found";
    }

}

This way you can have multiple controllers, each with multiple actions.

Final remarks.

  • On database side it would be handy to use one of existing ORM frameworks. They will save you days of work and will probably perform better than hand-crafted db tier. I would also suggest handling instantiation of PDO instance as instantiating PDO in every Model object is not the cleanest way. Something like DBFactory::getConnection would do.

  • You may consider returning HTML rather than echoing it out in controller. This gives you grat flexibility if you want to implement Intercepting Filters that would wrap controller, intercept its output and pre- or post-process it. It is super handy to have a filter that attaches HTML header and footer automatically.

  • Creating custom framework is great fun and valuable educational experience, however I would suggest using one of existing frameworks for more serious tasks.

All the best.

like image 123
Michał Niedźwiedzki Avatar answered Oct 25 '22 07:10

Michał Niedźwiedzki