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.

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>
share|improve this question

1 Answer 1

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.

share|improve this answer

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.