HiveBrain v1.2.0
Get Started
← Back to all entries
patternphpMinor

Correct MVC pattern implementation for form validation

Submitted by: @import:stackexchange-codereview··
0
Viewed 0 times
mvcvalidationformforcorrectimplementationpattern

Problem

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


Test

    Name 
    Parent 
    

getStatus() == 1): ?>
    Inserted Sucess
getStatus() == 0) : ?>
    Null fields


Category.php

```
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->prepa

Solution

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.

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


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 th

Code Snippets

application
    config
    models
    views
    controllers
data
    cache
    logs
public
    js
    css
    images
<?php
require 'classes/Database.php';
require 'clases/CategoryControl.php';
$category = new CategoryControl( $database );
$category->render( $view );
?>
//In Controller
public function render( $view ) {
    extract( $this->data );
    include $view;
}

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

Context

StackExchange Code Review Q#15530, answer score: 6

Revisions (0)

No revisions yet.