0

I'm trying to figure out if my class variables are looking correct and I'm using them properly. As with another question is I have some code that is used in multiple places so I'm trying to figure out what I can move to my Admin_Controller and make it into a function.

<?php
if (!defined('BASEPATH'))
    exit('No direct script access allowed');
class Dashboard extends Admin_Controller {
    protected $current_user;
    protected $data;
    public function __construct() {
        parent::__construct();
        $this -> load -> model('user_model', 'user');
        $user_id = $this -> session -> userdata('user_id');
        if ($user_id == FALSE) {
            redirect('login', 'refresh');
        }
        else {
            if ((!is_numeric($user_id)) || (strlen($user_id) < 5)) {
                $this -> session -> unset_userdata('user_id');
                $this -> session -> sess_destroy();
                redirect('login', 'refresh');
            }
            else {
                $this -> current_user = $this -> user -> get($user_id);
                if (!is_object($this -> current_user)) {
                    $this -> session -> unset_userdata('user_id');
                    $this -> session -> sess_destroy();
                    redirect('login', 'refresh');
                }
                else {
                    $this -> data['current_user'] = $this -> current_user;
                }
            }
        }
    }

    public function index() {
        if ($this -> current_user -> user_role_id >= 4) {
            $dashboard = 'admin_dashboard';
        }
        else {
            $dashboard = 'user_dashboard';
        }
        $this -> template -> set_theme('saturn') -> set_layout('default', 'admin') -> set_partial('header', 'admin/partials/header') -> set_partial('navigation', 'admin/partials/navigation') -> build('admin/' . $dashboard, $data);
    }
}
Improve this question
4
  • I don't understand, what exactly is your question? Aside from "does my code look ok?" Commented Feb 10, 2014 at 23:41
  • Am I using class variables correctly? Commented Feb 10, 2014 at 23:49
  • __construct()'s should have no side effects / actions IMHO. new DashBoard() causes a redirect? I would not like working with that code. Commented Feb 10, 2014 at 23:50
  • Is there something better I could do with it then? Commented Feb 10, 2014 at 23:52

2 Answers 2

Reset to default
0

//You can make your two class variable to public 

public $current_user;
public $data;

//move your function to the user_model

public function set_user_data() {

    $CI = & get_instance();

    $user_id = $this->session->userdata('user_id');
    if ($user_id == FALSE) {
        redirect('login', 'refresh');
    }
    else {
        if ((!is_numeric($user_id)) || (strlen($user_id) < 5)) {
            $this->session->unset_userdata('user_id');
            $this->session->sess_destroy();
            redirect('login', 'refresh');
        }
        else {
            $current_user = $this->user->get($user_id);
            if (!is_object($current_user)) {
                $this->session->unset_userdata('user_id');
                $this->session->sess_destroy();
                redirect('login', 'refresh');
            }
            else {

                $CI->data['current_user'] = $current_user; // why do you need to add same data in two variable
                $CI->current_user = $current_user; // you can use this variable any where in CI using get_instance()
            }
        }
    }
}
Improve this answer
Sign up to request clarification or add additional context in comments.

2 Comments

You don't need the reference in $CI = & get_instance();. Since PHP 5.0 all objects are passed by-ref anyway unless explicitly cloned using the clone keyword. This is a left-over relic from the PHP 4.2/4.3 times. Other than that, I would strongly suggest keeping $data protected and not public. Internal data should be kept internal and not be open to the public to do whatever they want with it.
If you move the function to a model and dont use $CI = & get_instance(); how you will set the controller variable? and yes i agree data should be protected and it should not set form the function set_user_data
0

keeping in mind this all just style etc -- but this is what i would suggest 

public function __construct() {

    parent::__construct();

    $this->load->model( 'user_model', 'user' );

    if ( ! $this->dashboardUser = $this->user->_verifyAndReturnUser() ) {

        redirect( 'login', 'refresh' ); }

}

// in your user model
function _verifyAndReturnUser() {

    if ( ! $user_id = $this->session->userdata( 'user_id' ) ) {
        $this->session->sess_destroy(); 
        return false ;  }

    elseif ( ! $user = $this->_getUserBy( $user_id )  ) {
        $this->session->unset_userdata( 'user_id' );
        $this->session->sess_destroy();
        return false ;  }

    else { return $user ;  }

}


// do this inside your controller methods (not the constructor)
$data['dashboardUser'] = $this->dashboardUser;

Use the database option for your sessions that way it won't write everything to the clients cookie. by putting the session check in your users model you can easily call it from other controllers. also keeps your constructor tidy.

when you are doing if / elseif / else -- by checking for the "not" condition first - you can keep your if checks simpler and one level deep.

the controller method should determine exactly what is going to the views. different methods will have different requirements. hence suggesting: $data['dashboardUser'] = $this->dashboardUser; in the method.

notice how in the model method i kept it generic and called the value $user… and then in the 'dashboard' controller i called it "$dashboardUser" and passed it to $data that way . so then in your view when you see references to $dashboardUser -- its going to be very clear what kind of user it is, and what controller it came from.

Improve this answer

Comments

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service and acknowledge you have read our privacy policy.