Take the 2-minute tour ×
Code Review Stack Exchange is a question and answer site for peer programmer code reviews. It's 100% free, no registration required.

Before going out and learning a full fledged framework I'm trying to understand the MVC pattern coding basic stuff, at the moment I'm testing with MVC applied to form validation.

After reading a good amount of articles and code examples I came out with my own version, nothing too elaborate as it's mostly for learning, so I need advice in the structure.

For this example I have a basic form which inserts a Category into a database, so I divided it like this:

  • class Category extends Operations: This class only deals with database management stuff CRUD operations (the business logic), defines which fields are required and the table name. I call this my model.
  • abstract class Operations: Will be extended by other classes that I'll make in the future such as Products, Clients, etc. Contains common properties (id, attributes). Again part of the model.
  • class CategoryControl: The controller, deals with $_POST data, setting the corresponding attributes for the Category object and gets its status (insert success/failed, for example).
  • abstract class Controller: Generic class which contains form submit status and executes the corresponding CRUD operation.
  • Finally the view which is the form, only requesting for the object status to display a sucess/failure message to the user.

Here's the code for each:

view.php:

<?php 
require('clases/Database.php'); 
require('clases/CategoryControl.php');
$category = new CategoryControl($database);
?>

<!DOCTYPE html>
<html>
<head>
<title>Test</title>
<meta charset="utf-8" />
</head>
<body>

<form action="view.php" method="post">
    <label>Name </label><input type="text" name="name" maxlength="80" /><br />
    <label>Parent </label><input type="text" name="parent" maxlength="80" /><br />
    <input type="submit" name="submit" value="Insert" />
</form>

<?php
if ($category->getStatus() == 1): ?>
    <p>Inserted Sucess</p>
<?php
elseif ($category->getStatus() == 0) : ?>
    <p>Null fields</p>
<?php
endif; ?>

</body>
</html>

Category.php:

<?php
require("Operations.php");

class Category extends Operations {     
const TABLE = 'category';   
private $reqFields = array('name', 'parent'); 

public function __construct($pdo_link) {
    parent::__construct($pdo_link);
}

public function insert() {
    if ($this->checkNullFields($this->reqFields)) {         
        $stmt = $this->database->prepare("INSERT INTO ". self::TABLE ." (name, parent) VALUES (:name, :parent)");

        foreach ($this->attribs as $field => $value) 
            $stmt->bindValue(':'.$field, $value);

        $stmt->execute();
        $stmt = NULL;           

        return true;
    } else          
        return false;
}

public function update() {
    if ($this->checkNullFields($this->reqFields)) {         
        $stmt = $this->database->prepare("UPDATE ". self::TABLE ." SET name = :name, parent = :parent WHERE id = :id");

        foreach ($this->attribs as $field => $value) 
            $stmt->bindValue(':'.$field, $value);

        $stmt->bindValue(':id', $this->id);                 
        $stmt->execute();

        $stmt = NULL;           

        return true;
    } else          
        return false;
}

public function delete() {
    if (!empty($this->id)) {
        $stmt = $this->database->prepare("DELETE FROM ". self::TABLE ." WHERE id = :id");       

        $stmt->bindValue(':id', $this->id);
        $stmt->execute();

        $stmt = NULL;

        return true;
    } else
        return false;
}   
}
?>

Operations.php:

<?php
abstract class Operations {     
protected $database; // PDO link

public $id; 
public $attribs;
public $status; 

protected function __construct($pdo_link) {
    $this->database = $pdo_link;
}

abstract protected function insert();
abstract protected function update();
abstract protected function delete();

protected function checkNullFields($reqFields) {    
    foreach ($reqFields as $i => $value) {
        if (empty($this->attribs[$value]))
            return false;       
    }   

    return true;
}
}
?>

CategoryControl.php:

<?php
require('Category.php');
require('Controller.php');

class CategoryControl extends Controller {      
public function __construct($pdo_link) {        
    $this->model = new Category($pdo_link);

    if ($this->checkSubmit()) {
        $this->initAttribs();
        $this->processPostData();
    }
}

public function initAttribs() { 
    $this->model->id = isset($_GET['id']) ? $_GET['id'] : '';

    $this->model->attribs = array(
                        'name' => isset($_POST['name']) ? $_POST['name'] : '',
                        'parent' => isset($_POST['parent']) ? $_POST['parent'] : ''
                        );
}
}


?>

Controller.php:

<?php
abstract class Controller {
protected $model;

protected function checkSubmit() {
    if (isset($_POST['submit'])) {
        return true;
    } else {
        $this->model->status = -1;
        return false;
    }
}

protected function processPostData() {      
    if ($_POST['submit'] == 'Insert')
        $this->model->status = $this->model->insert();
    else if ($_POST['submit'] == 'Update')
        $this->model->status = $this->model->update();
    else if ($_POST['submit'] == 'Delete')
        $this->model->status = $this->model->delete();  
}

abstract protected function initAttribs();

public function getStatus() { return $this->model->status; }
}
?>
share|improve this question
add comment

1 Answer

up vote 2 down vote accepted

Directory Structure

When implementing a heavy design pattern like MVC, the best thing to do is set up your directory structure to support it. For example, you currently have, at the very least, your Model and Controller in the same directory. When dealing with just a few files this may seem fine, but once you start expanding you will soon realize that this will quickly become a mess. Look around at how different MVC frameworks set up their directory structure and adapt yours to whichever fits your needs. For example, I've adapted something pretty similar to Zend's, which looks something like this:

application
    config
    models
    views
    controllers
data
    cache
    logs
public
    js
    css
    images

Not saying you have to adopt this structure, just saying its the one I like and have found to meet all of my needs. You can also create your own. Don't limit yourself to prefab solutions, but don't dismiss them either. There was a lot of thought put behind those solutions, so they are likely to be pretty efficient.

Views and Index

Your Views should not be knowledgeable of the type of framework you are using. This makes it easier to change the framework should it ever become necessary. So, including the Model and Controller in the View is the wrong way to go about this. A View should only have simple templating variables, and maybe a header or footer inclusion.

Remember, the index is not a view. It is the first page of your website and should be the page all requests are made from. The Controller should be included in the index via some get variable, or some default value when first run. The Model is always associated with the Controller, so it is the Controller's job to load it, not the index's or the View's. So, a link to mywebsite.com should result in a default Model, Controller, and View being loaded, or just a Controller or View, depending on what default page or implementation you end up using. Similarly, a link to mywebsite.com?controller=Controller&view=View or rewritten to use modrewrite (think its modrewrite) as mywebsite.com/Controller/View should load the correct Controller and pass the View to that Controller to be loaded. So your index should look something like this.

<?php
require 'classes/Database.php';
require 'clases/CategoryControl.php';
$category = new CategoryControl( $database );
$category->render( $view );
?>

Notice the render method. It doesn't have to be called render, but typically a controller has such a method that extracts all of the template data into the local variable scope and includes the view. This is the implementation I use, but it is not the only one. Some people don't like using extract() and instead use a foreach loop to make variable-variables, some use the class implementation as you are, but I find the extract cleaner. However, I would stick to one of the first two implementations. Using a class instance requires a dependency on the framework, which we've already established was bad, and makes your code less non-PHP user friendly. Meaning a pure HTML programmer will have some difficulties interfacing the PHP and HTML. Its better to abstract those class methods to variables to help those non-PHP users and because it just cleans up the HTML. Even if you aren't in a group, it is still a good idea to program like you are. This gets you in the right frame of mind for future group projects, and typically makes your code more reusable.

//In Controller
public function render( $view ) {
    extract( $this->data );
    include $view;
}

//In View
<?php if( $status == 1 ) : ?>

By the way, where did $database come from? You just dumped it into the category constructor without defining it. Normally I would assume this came from the controller, but you don't appear to be following this method. I would hate to assume this is a global.

Models

The structure of your Models are fine, but there are a few of things I would like to point out.

First, you don't need to redeclare a method that has been defined in a parent class unless you are planning on doing something extra with it. Inheritance declares that any non-private methods and properties are automatically included in any children without the express need of declaring it so. In other words, defining a constructor in a child just to declare it to use the parent constructor is redundant. If you did something before or after calling the parent constructor, then that would be different, that's a form of polymorphism.

The second thing is that abstract classes cannot be instantiated, therefore they should not, and can not have constructors. You are likely getting warnings about this in the background. Instead use an init() method and in each child constructor call it, or explicitly define the constructors in the child classes.

Another thing I want to point out, though I'm sure some will disagree, is that you shouldn't use the "braceless" syntax. PHP inherently requires braces for its statements. It allows you to ignore this requirement so long as you promise not to use more than one line. But that's where it ends. It would be different if there was not that limitation, but that's not the case. Once you have two lines, you have to go back and add those braces, and this is where many people make mistakes. Besides, having to go back and add those braces just distracts from the task at hand. I would much rather just add them to begin with and later only have to start typing. PHP , in my opinion, should not allow this syntax unless they plan to change their syntax to allow for full bracless coding. Sorry for the mini rant, I just figured I'd do my best to warn you off of it. If you like braceless code, try Python :)

There is no need to NULL out a variable, or anything really. This kind of micro-memory management just leads to sloppy code and the minute increase in performance is so negligible as to not be worth it. I know some people have been pushing this on reddit and in some blog posts, but the benefits do not outweigh the costs.

Your insert() and update() methods are violating the "Don't Repeat Yourself" (DRY) Principle. As the name implies, you can combine these methods into a third shared method. Then each specific method would pass the proper MySQL statement to the shared method. The only catch would be adding the "id" to the $attribs property before running the shared method so that it will be bound, and you may need to remove it after. But this is still better.

Controllers

The structure of your Controllers are also fine. There's only one thing I would like to point out here: Sanitize your user input. You are validating it, at least in as much as you are ensuring it exists, but you don't validate it. If your PHP version is >= 5.2, you can check out filter_input() function. This also makes checking if an index is set unnecessary as it will return FALSE if not.

All in all, I'm very impressed. The implementation is sound and just needs some minor tweaks. Most people don't take to it so quickly. I had a whole disclaimer about how MVC was hard to learn but I had to remove it because you didn't need it :P Anyways, I hope this helps. If you have any questions, just leave me a comment.

share|improve this answer
    
That's what I call an asnwer! Nice piece of advice there, I sure will be re-reading this from time to time. Now about the directory structure I already had in mind something like that, that was basically a stub. The thing i still find difficult to grasp is the single entry point, I guess it's just matter of more reading and practice, can't get it right at the first try. About $database, yes it's a global instance of the PDO class which is inside the 'Database.php' code and every page includes it (I found it easier to implement it that way while I learn the single entry point concept. –  user1647798 Sep 12 '12 at 21:06
    
About the model, i recognize there are indeed some redundancies in the CRUD methods, in fact I first programmed them like you mentioned (a general insert/upd method), but then changed it since I found it kinda clunky (as you said, separating the ID from the attribs, etc). About the php braces, I've found myself adding braces now and then since I'm too lazy to add braces for one liners which then grow to be more complicated, it's just matter of discipline. About the micro-optimizations, it's a result of reading a lot of opinions, i guess php garbage collector takes care of those trivial things. –  user1647798 Sep 12 '12 at 21:13
    
@user1647798: The single entry point is a bit hard to grasp. For a while there my mind refused to accept it was possible and I kept trying to do extra stuff on the index. I don't know what finally clicked, but something did, and its relatively simple once you get past the barrier. Anyways, don't use globals, they are evil and should be completely removed from the language. If you need to keep a single instance, do so through injection. In this case, the first time you should need it is in the index when injecting it into the controller. Just create it then. –  mseancole Sep 12 '12 at 21:15
    
I'm going slow and steady, I programmed a website procedurally and just recently I've started to learn the OOP paradigm and MVC to rewrite it, since It's a nightmare to maintain a site coded like that. All in all thanks for taking the time to formulate that answer, I appreciate it. –  user1647798 Sep 12 '12 at 21:17
add comment

Your Answer

 
discard

By posting your answer, you agree to the privacy policy and terms of service.

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