5
\$\begingroup\$

I've tried to improve my code: separate HTML/PHP, much clear, next time change will be easier. After doing research on MVC/OOP, I've made the below code to learn.

I understand this is not the MVC pattern now. Can anybody help me fix it to become actual MVC?

  1. get the result from db (list all)
  2. get the result from db (list search)

index.php

require_once 'grant.php';
require_once 'controller.php';
$controller = new controller();
$controller -> temp_index();

controller.php

require_once 'model.php';
class controller{
    public $model;
    public function __construct(){
        $this -> model = new model();
    }

    public function temp_index(){
        require 'temp_index.php';
        if($_REQUEST['submit'] == 'get_all'){
            $result = $this -> model -> get_all();
            require 'get_all.php';
        }
        else if($_REQUEST['submit'] == 'get_search'){
            $result = $this -> model -> get_search();
            require 'get_search.php';
        }
    }
}

model.php // please ignore not use PDO

class model{
    public function get_all(){
        $sql = "select * from tb order by id desc";
            $query = mysql_query($sql) or die(mysql_error());
            $result = array();
    while($list = mysql_fetch_array($query)){
                $result[] = $list;
            }
            return $result;
    }
    public function get_search(){
        $search = mysql_real_escape_string($_POST['search']);
        $search = trim($search);
        if($search !== ''){
            $sql = "select * from tb where ac_email like '%$search%'";
            $query = mysql_query($sql) or die(mysql_error());
            if(mysql_num_rows($query) > 0){
                $result = array();
                while($list = mysql_fetch_array($query)){
                    $result['result'][] = $list;
                }
                return $result;
            }
            else{
                $result['statu'] = 'can\'t find search';
                return $result;
            }
        }else{
            $result['statu'] = 'can\'t find search plz input text';
            return $result;
        }           
    }
}

get_search.php // view

<div class="reslt_get_search">
<?php foreach ($result['result'] as $list) : ?>
    <div><?php print"$list[ac_id]";?></div>
    <div><?php print"$list[ac_email]";?></div>
<?php endforeach; ?>

    <div><?php print"$result[statu]";?></div>
</div>

temp_index.php // view

<form action="" method="POST">
    <input type="submit" name="submit" value="get_all">
</form>
<form action="" method="POST">
    <input type="text" name="search">
    <input type="submit" name="submit" value="get_search">
</form>
\$\endgroup\$
3
\$\begingroup\$

SQL Injection

You don't have to use PDO, but you have to use something to defend against SQL injection, and mysql_real_escape_string isn't it (for one because MySQL is deprecated, and also because it's way too easy to forget to put ' around the variable in the query, and once it's forgotten it's hard to see that it's missing). Using prepared statement isn't hard (see here for mysqli and here for PDO).

Early Return

If you return early, you can often reduce the nesting of if statements:

if($search == ''){
    $result['statu'] = 'can\'t find search plz input text';
    return $result;
}
$sql = "select * from tb where ac_email like '%$search%'";
$query = mysql_query($sql) or die(mysql_error());

if(mysql_num_rows($query) < 0){
    $result['statu'] = 'can\'t find search';
    return $result;
}
[...]

Error Messages

I wouldn't just echo mysql_error, as it's probably not that informative for the user, and it might reveal information. Just write a custom error message.

Your current error messages should also be better:

  • can\'t find search plz input text: could be You did not enter any Search terms.
  • can\'t find search: could be No results found for your Search term.

Naming

  • classes should start with an uppercase letter.
  • temp_index.php: search_form.php would be better.
  • get_search.php: could be search_result.php.
  • statu isn't that much shorter than status, just write it completely.

Misc

  • make your model field private.
  • be explicit in your request type. If you send post data, don't just get request, but post.
  • don't just die in a model, throw an exception and let the controller handle it (ideally by passing a custom error message to the view).
  • select * is discouraged, just select what you actually need.
  • use more spaces (after print, and after )).
  • ac_email seems user supplied, so I would filter it with htmlspecialchars before echoing (because of XSS; I hope that you cleaned the email address before inserting it in the database, but you can never be too save).

MVC

There is more than one definition of MVC out there, and especially in web applications the definitions are not all that clear (you can see this if you just do a quick google search, there are a lot of different diagrams out there). For example, the original approach of the model notifying the view of changes in the underlying data (see graphic here) was abandoned by (most or all) web application frameworks (see graphic here).

I would say that your solution is pretty much complying with the (or better an) MVC approach. You have a light-weight controller which processes user input, you have a view which reacts to the controller, and you have a model.

What you should not do is access $_POST in the model. Just pass it to the model from the controller. And as mentioned above, your model also shouldn't just die.

You could also create a class for the actual data instead of using arrays.

\$\endgroup\$
3
\$\begingroup\$

Object Oriented Programming Improvements

The first thing you can do to improve code like this is to use dependency injection, instead of instantiating objects with new inside of objects.

// Setup your class hierarchies like this.

abstract class Controller
{
    protected $model;

    protected function __construct(Model $model)
    {
        $this->model = $model;
    }
}

class RootController extends Controller // or HomeController extends Controller
{
    public function __construct(Model $model)
    {
        parent::__construct($model);
    }
}

Then you execute:

$controller = new RootController(new RootModel());  // But, read on.

Notice the use of type hinting in the __construct function. Program to an interface, not an implementation.

Use abstract classes (which you cannot make an instance of) to factor out common properties and methods into one place. Extend abstract classes with specific species of classes. This is where the power of polymorphism via the Strategy Pattern comes in to play!

By type hinting with abstract classes, or interface types, you will not have to change your code simply because a different species of child object need to be used at some other point in time.

If you find that the parents of several classes should have the same functionality or constants, factor again and define a PHP Interface and slap it on to a class like this.

class RootController extends Controller implements Printable
{
    public function __construct(Model $model)
    {
        parent::_construct($model);
    }
}

Where in this case, Printable would be a set of virtual functions, and or constants, that form a contract, forcing any class that implements Printable to define certain public functions / hooks so that it can be used as a "Printable" object by unrelated code in other places.

Model-View-Controller Improvements

In terms of MVC, you may want to consider how a Model is going to send data to a View. Using a Controller's method as the working area for passing dat from a Model to a View is common. If a Controller's method can act as a working space for a particular command, then "M" and "V" (Model and View) can use "C's" method as a place to coordinate things.

Given the following, you could use your programming knowledge to devise a solution where a View is passed data from a Model, without injecting an instance of said Model into a View. In other words, you do not want to do this.

abstract class View
{
    protected $model;

    protected function __construct(Model $model)
    {
        $this->model = $model;
    }
}

class NewsletterView extends View
{
    public function __construct(Model $model)
    {
        parent::__constrcut($model);
    }
}

Summarizing, there are two well known options:

  1. Use a method of a Controller (i.e. a command). Have the Model pass a data structure (array or iterable object) to the View.

  2. Give the View a reference to the Model and delegate the chore of getting data to the Model within the View. However, as noted this is not what you want to do.

Note that option #2 can create strong coupling between a Model and a View, because the View must then use methods off of the Model instance to get at data. If a Model's methods change (such as the names, or method parameters), the View will have to change, accordingly.

What you want to do is have a setup like this.

abstract class Controller
{
    protected $model;
    protected $view;

    protected function __construct(Model $model, View $view)
    {
        $this->model = $model;
        $this->view = $view;
    }
}

class NewsletterController extends Controller // or HomeController extends Controller
{
    public function __construct(Model $model, View $view)
    {
        parent::__construct($model, $view);
    }

    /**
    * A totally made up method.
    */
    public function register(array $suscriberData) 
    {
        $viewData = $this->model->addSubscriber($suscriberData));
        $this->view=>updateRegistrationPage($viewData);
    }
}

Then you execute:

$controller = new NewsletterController(new NewsletterModel(), new NewsletterView());

(Note: A dependency injection container can resolve class dependencies off screen and just issue you the object you request! Very handy, but not mandatory.)

Your URL to such a command would be:

http://yourdomain.com/newsletter/register

Where "newsletter" is your Controller, and "register" is the command.

Typically, the machinery of a framework (Front Controller / Router / Dispatcher), or just good programming generally, handles transforming the path component of a URL (REQUEST_URI) into an instance of a Controller. In that case, because instances are being created dynamically, you will want to use a class autoloader (as in PHP-FIG PSR-4) at minimum. Using an autoloader can eliminate all but one require statement for accessing classes. You will need one require statement to bring your autoloader code into scope. :-)

This is why people are encouraged to use namespaces with their OO PHP code. Using a PSR-4 compliant autoloader will automatically find and load your custom classes! The implication being, that your include_path no longer needs to list every dog-on path that you have classes loading from! ;-)

However, if you use modular HTML templates for your web pages (require header.php, require footer.php, require aside.php, etc ..), be sure to have the location of those files in your include_path!

Dependency injection containers are not mandatory, but they do simplify the usage of objects by decoupling where a class is instantiated from where it is used. In other words, a DI container is a very precise gumball machine for objects and other data. :-)

\$\endgroup\$
  • \$\begingroup\$ There is more than one implementation of an MVCish kind of software architecture. Many people use frameworks that adulterate the system. Without bowing to the frameworks and assuming people have to use them; or assuming that the frameworks are right to do it differently; a model that updates a view is the reference we can rely on to have a meaningful conversation \$\endgroup\$ – Anthony Rutledge Apr 30 at 12:46

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.