Design pattern usage - refactoring an old code in CI

Question

everyone! I'm rewriting a god object with lo-ong methods into something much more viewable and pretty.

However, I think there is something missing and can be done better. So I'm sending examples of one controller and two models in order to get an idea.

Any constructive criticism, notes and improvement suggestions would be very helpful!

Thanks in advance.

controller Account.php

<?php

    if ( ! defined('BASEPATH')) exit('No direct script access allowed');

    /**
     *
     * handle accounts transformations
     */
    class Account extends CI_Controller {

        function __construct()
        {
            parent::__construct();
            $this->load->model("outputModel");
            $this->load->model("userModel");
            $this->load->model('validation');
            $this->load->database();
        }

        /**
         * login
         *
         * @post email
         * @post password
         * @post device_token optional
         * @post device_family optional
         *
         * @return OutputModel $outputModel auth_token
         */
        function index() {

            $email = $this->input->post('email');
            $password = $this->input->post('password');

            if (isset($_POST['device_token']) && isset($_POST['device_family'])) {
                $device_token = $this->input->post('device_token');
                $device_family = $this->input->post('device_family');

                $result = $this->userModel->Login($email,$password,$device_token,$device_family);
            } else {

                $result = $this->userModel->Login($email,$password);
            }

            $this->outputModel->process($result);

            echo $this->outputModel;
        }

        /**
         * get unread messages and groupchats from (last) dated up to limit
         *
         * @post string $auth_token
         * @post int $limit amount of groupchats requested
         * @post int $date_up_to unixtime optional
         * @post boolean $order_by_date_desc optional
         *
         * @return OutputModel $outputModel echo jsoned
         */
        function refresh() {

            $token = $this->input->post('auth_token');
            $data['user_id'] = $this->validation->checkSession($token);

            $data['limit'] = $this->input->post('limit');

            if (isset($_POST['date_up_to']))
                $data['date_up_to'] = $this->input->post('date_up_to');
            if (isset($_POST['order_by_date_desc']))
                $data['order_by_date_desc'] = $this->input->post('date_up_to');


            $result = $this->groupChatModel->getUnread($data);

            $this->outputModel->processResult($result);

            echo $this->outputModel;

        }


        function activate($activation_key) {

            if (!$this->validation->checkSHA256($activation_key)) {

                $this->load->view("activate_unsuccessful.php");
                return;

            } else {
                //continue
            }

            $this->load->library('email');
            $this->load->helper('url');
            $this->load->database();
            $this->load->helper('string');

            # check account exists
            $sql = "SELECT * FROM user WHERE activation_key = ?";
            $query = $this->db->query($sql, array($activation_key));
            if ($query->num_rows() > 0) {

                # activate account
                $sql = "UPDATE user SET activation_key = 0 WHERE activation_key = ?";
                $this->db->query($sql, array($activation_key));

                $this->load->view("activate_successful.php");
            } else {
                $this->load->view("activate_unsuccessful.php");
            }
        }

        /**
         * creates account
         *
         * @post string first_name
         * @post string last_name
         * @post string phone
         * @post string email
         * @post string password
         *
         * @return OutputModel $outputModel auth_token on success or error
         *
         */
        function create()
        {
            $this->load->model('userModel');
            $first_name = $this->input->post('first_name');
            $last_name = $this->input->post('last_name');
            $phone = $this->input->post('phone');
            $email = $this->input->post('email');
            $password = $this->input->post('password');
            $result = $this->userModel->CreateAccount($first_name, $last_name, $phone, $email, $password);

            $this->outputModel->process($result);

            echo $this->outputModel;

        }

        /**
         * changes setting of an account
         *
         * @post string auth_token
         * @post string new_first_name
         * @post string new_last_name
         * @post string new_phone
         * @post string new_password
         * @post string old_password
         *
         *
         * @return OutputModel $outputModel auth_token on success or error
         */
        function changeSettings() {

            $token = $this->input->post('auth_token');
            $data['user_id'] = $this->validation->checkSession($token);

            $data['new_first_name'] = $this->input->post('new_first_name');
            $data['new_last_name'] = $this->input->post('new_last_name');
            $data['new_phone'] = $this->input->post('new_phone');
            $data['new_password'] = $this->input->post('new_password');
            $data['old_password'] = $this->input->post('old_password');
            $result = $this->userModel->ChangeSettings($data);

            $this->outputModel->process($result);

            echo $this->outputModel;
        }

    }

models 1.Usermodel

<?php
if ( ! defined('BASEPATH')) exit('No direct script access allowed');

class UserModel extends CI_Model {


    /**
     * @param $email
     * @param $password
     * @param null $device_token
     * @param null $device_family
     * @return array
     */
    public function Login($email, $password, $device_token = null, $device_family = null )
    {

        $sql = "SELECT * FROM user WHERE (email = ?) LIMIT 1";
        $query = $this->db->query($sql, array(strtolower($email)));

        if (!($query->num_rows() > 0)){
            $error = #some code error
            return array('error' => $error);

        } else {
            //continue
        }

        $row = $query->row();

        if ($row->password != encrypt($password) {
            $error = #some code error
            return array('error' => $error);

        } else {
            //continue
        }

        # Create a session id and store it in session table with device token and device family
        $user_id = $row->id;
        $token = hash(func,uniqid());
        $expire_date = time() + (24*60*60); //now + 24 hours
        $sql = "INSERT INTO auth_tokens (auth_token, user_id, expire_date) VALUES (?, ?, ?)";
        $this->db->query($sql, array($token, $user_id, $expire_date));

        # Create entry in devicetoken table
        if ($device_token && $device_family) {
            $this->AddDevice($user_id, $device_token, $device_family);
        }

        # put session id and user id into response
        $response['auth_token'] = $token;
        $response['id'] = $user_id;
        $response['first_name'] = $row->first_name;
        $response['last_name'] = $row->last_name;

        return array('result' => $response);
    }

    /**
     * @param $user_id
     * @param $device_token
     * @param $device_family
     * @return void
     */
    public function AddDevice($user_id,$device_token,$device_family) {

        $sql = "SELECT * FROM devicetoken WHERE device_token = ? AND device_family = ?";
        $query = $this->db->query($sql, array($device_token, $device_family ));

        if (!$query->num_rows() == 0) {
            $sql = "UPDATE devicetoken SET user_id = ?, device_family = ? WHERE device_token = ?";
            $this->db->query($sql, array($user_id, $device_family, $device_token));
        } else {
            $sql = "INSERT INTO api_devicetoken(user_id,device_token,device_family) VALUES (?,?,?)";
            $this->db->query($sql, array($user_id,$device_token,$device_family));

        }

        return;
    }

    /**
     * @param array $data
     * @return array
     */
    public function ChangeSettings(array $data) {

        $this->load->model('validation');

        $sql = "SELECT * FROM user WHERE id = ?";
        $query = $this->db->query($sql, $data['user_id']);

        if ($query->num_rows() == 0) {
            return array('error' => #some code error);
        }

        $row = $query->row();

        //check password
        if ($row->password!=encrypt($data['old_password'])) {
            return array('error' => #some code error);
        }

        //(process|substitute old) first name
        if (array_key_exists('new_first_name',$data)) {
            if (!$this->validation->checkName($data['new_first_name'])) {
                return array('error' => #some code error);
            }
        } else {
            $data['new_first_name'] = $row->first_name;
        }

        //(process|substitute old) last name
        if (array_key_exists('new_last_name',$data)) {
            if (!$this->validation->checkName($data['new_last_name'])) {
                return array('error' => #some code error); 
            }
        } else {
            $data['new_last_name'] = $row->last_name;
        }

        //(process|substitute old) phone
        if (array_key_exists('new_phone',$data)) {
            if (!$this->validation->checkName($data['new_phone'])) {
                return array('error' => #some code error); 
            }
        } else {
            $data['new_phone'] = $row->phone;
        }

        //(process|substitute old) new password
        if (array_key_exists('new_password',$data)) {
            if (!$this->validation->checkPass($data['new_password'])) {
                return array('error' => #some code error); 
            }
        } else {
            $data['new_password'] = $data['old_password'];
        }

        $data['new_password'] =  encrypt($data['new_password']);


        $sql = "UPDATE user SET first_name = ?, last_name = ?, password = ? WHERE id = ?";
        $this->db->query($sql, array($data['new_first_name'],$data['new_last_name'],
                                     $data['new_password'],$data['user_id']));

        if (!$this->db->affected_rows()) {
            return array('error' => #some code error); 
        }

        // update phone
        $sql = "UPDATE api_user_data SET value = ?, hash = ? WHERE user_id = ? AND type_id = 3";
        $this->db->query($sql, array($data['new_phone'], hash($data['new_phone']), $data['user_id']));

        unset($data['new_password']);
        unset($data['old_password']);

        return array('result' => $data);
    }

    /**
     * @param $first_name
     * @param $last_name
     * @param $phone
     * @param $email
     * @param $password
     * @return array
     */
    public function CreateAccount($first_name, $last_name, $phone, $email, $password)   {

        $this->load->model('validation');

        if (!$this->validation->checkName($first_name))
            return array('error' => #some code error);

        if (!$this->validation->checkName($last_name))
            return array('error' => #some code error);

        if (!$this->validation->checkPhone($phone))
            return array('error' => #some code error);

        if (!$this->validation->checkEmail($email))
            return array('error' => #some code error);

        if (!$this->validation->checkPass($password))
            return array('error' => #some code error);

        # Check if email address exists: error = #some code error if yes
        # If email exists, but not verified - resend verification is to be used
        $sql = "SELECT * FROM user WHERE email = ?";
        $query = $this->db->query($sql, array($email));

        if ($query->num_rows() > 0)
            return array('error' => #some code error); //user already exists


        # create activation key
        $key = encrypt(uniqid());
        # prepare password
        $password = encrypt($password);

        # store user details with activation key
        $sql = "INSERT INTO user (first_name, last_name, password, user_type, activation_key) VALUES (?,?,?,?,?)";
        $query = $this->db->query($sql, array($first_name, $last_name, $password, 1, $key)); //1 means type of user
        if ($query) {
            $user_id = $this->db->insert_id();

            $sql = "INSERT INTO user_data(user_id,type_id,value,hash) VALUES (?,?,?,?),(?,?,?,?)";
            $this->db->query($sql, array($user_id, 1, $email, hash($email),  //1 - main email user data type
                                         $user_id, 3, $phone, hash($phone)));//3 - phone user data type
        } else
            return array('error' => #some code error); //problem updating database

        # send email with activation link
        $this->SendActivationEmail($email,$key);

        return array('result_object' => array('user_id' => $user_id));
    }

    /**
     * @param $email
     * @param $key
     * @return void
     */
    public function SendActivationEmail($email,$key)    {

        $this->load->library('email');
        $this->load->helper('url');

            $activation_url = site_url("/$some_path/$key");
            $this->email->from('some adress');
            $this->email->to($email);
            $this->email->subject('activation');
            # if account is activated - $key is 0
            if ($key) {
                $this->email->message("Click here to activate your account: $activation_url");
            } else {
                $this->email->message("Account already has been activated.");
            }
            $this->email->send();

    }


}

2.OutputModel

<?php
if ( ! defined('BASEPATH')) exit('No direct script access allowed');

class OutputModel extends CI_Model {

    private $error  = array("error_object"=>array(
                                "error"=>0)
                            );
    private $result = array();

    function __construct() {
        // Call the Model constructor
        parent::__construct();
    }

    /**
     * @return string jsoned array of $error and $result
     */
    function __toString() {
        $output=array();
        foreach (array($this->error,$this->result) as $value) {
            foreach ($value as $subkey=>$subvalue) {
                $output[$subkey]=$subvalue;
            }
        }
        $output = json_encode($output, JSON_FORCE_OBJECT);
        return $output;
    }

    /**
     * process data into output object
     *
     * @param array $result "resultObject"
     * @return void
     */
    function process(array $data) {
        if (array_key_exists("result",$data)) {
            foreach ($data["result"] as $key=>$value) {
                $this->result["result_object"][$key]=$value;
            }
        }
        if (array_key_exists("error",$data)) {
                $this->result["error_object"]["error"]=$data['error'];
        }

    }

}

mseancole · Answer 1 · 2013-11-06 02:44:52Z

Disclaimer: I have never used CodeIgniter, so this will mainly focus on general PHP coding practice.

First of all no comments is better than misleading comments. Take a look at some of the online resources for doccomments. The @ declaration is used for parameters or class information, not internal variables. I've never even seen @post before and don't think it is recognized by any IDE. Besides, this information isn't important to the person using your code. They only need to know what is required, what it returns, if anything, and what it does. Comments for yourself should be done outside of the code so as not to clutter up your source, though if you use self-documenting code comments will be largely unnecessary.

There are better ways to prevent direct script access than checking if a constant is set. Check for a session or user credentials if you must, but the best way is to use an .htaccess file. If done correctly, the .htaccess rules can make it so that no page, other than the index, is directly accessible and all other requests are automatically forwarded back to the index. Admittedly, .htaccess isn't a cureall, AJAX, for instance, is still considered direct access because it is being done client side. This is where a good directory structure, strategic .htaccess files, and a session or credentials check can really shine. Lastly, even if they do somehow access this page directly, they will see nothing as it is a class and no output is directly generated.

if ( ! defined('BASEPATH')) exit('No direct script access allowed');

Two more things about the above snippet. Under normal circumstances exit() or die() are very inelegant ways of terminating a script. Its fine here because its a part of the site most people shouldn't see anyway, but its something to keep in mind. Finally, braceless syntax should be avoided. You are saving very little by neglecting your braces while also opening yourself up to potentially troublesome errors later. They take no extra effort to add in and will save you debugging later should you extend this code.

Declare your method access levels. PHP defaults to public, but you should not rely on this.

public function __construct() {

I'm not 100% on this, again, I've never used CI, but I believe if a framework provides an interface for calling GET/POST/COOKIES then it should do isset() checks and default values for you as well, otherwise, what's the point. So...

$device_token = $this->input->post('device_token');
$device_family = $this->input->post('device_family');
if( $device_token && $device_family ) {//login with token and family

However, if your model is built correctly, then the default values for the $device_token and $device_family parameters in the Login() method should be FALSE anyway, so that if/else statement is unnecessary. Declare it once and cover the default functionality in the method.

//defaults for the last two params should be handled in Login()
$result = $this->userModel->Login($email,$password,$device_token,$device_family);

You should remain consistent with your naming schemes. Your class names are capitalized, but the created objects are lowercase camelCase. Most of your methods are lowercase, but Login() is uppercase. You use both camelCase and under_scored variables. Remaining consistent in your naming scheme will help you identify types and write your code without having to continually look up how every new variable is done. I'm currently working on a project like this and I have to say it is the most frustrating thing I've ever done. Please be consistent. You will thank yourself later, and inheriting programmers will thank you.

Using the __toString magic method is a bit confusing. You should use render() or a similarly named method, though the model should not be in charge of the display (more on this in the section entitled MVC).

echo $this->outputModel;
//should be
$this->outputModel->render();
//if models were actually supposed to render that is

Else statements are unnecessary if you aren't actually using them. You are actually returning early in all of these else//continue statements, so an else statement isn't even necessary anyway, the remaining code is an assumed else.

if (!$this->validation->checkSHA256($activation_key)) {
    $this->load->view("activate_unsuccessful.php");
    return;//returned, so rest of code is "else"
}// else {
    //continue
//}

I stopped reading at about this point. This is already a lot to go through and there were a lot of repeat offenses. I'll wrap this up with a basic introduction to MVC.

MVC

This is the part that most people tend to struggle with when creating MVC frameworks. It is not the controller's or the model's jobs to display content; That is the view's responsibility. So echoing out the model is doubly wrong. Its wrong because the controller shouldn't echo anything; And its wrong because the model shouldn't have anything to echo. There are many different ways to do MVC, but the underlying principles remain the same. The controller collects potential data its views need from the model. The model stores the data and provides ways of accessing it.

This is a very simplified version of what one of my MVC frameworks looks like (from memory, so sorry its not exact):

$controller = new Controller();//init controller values
$controller->setModel( 'model' );//set model
$controller->render( 'view' );//prep page variables and include view

asked	1 month ago
viewed	70 times
active	1 month ago

Explore our sites

Design pattern usage - refactoring an old code in CI

1 Answer

Your Answer

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

Hot Network Questions

Explore our sites

Design pattern usage - refactoring an old code in CI

1 Answer

Your Answer

Sign up or login

Post as a guest

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

Related

Hot Network Questions