1
\$\begingroup\$

I want to write better code. This is a simple controller class for an administrator login.

Are there conventions or tips in PHP to rewrite this code and improve it?

<?php

class Administrators extends CI_Controller {

    public function __construct() {
        parent::__construct();
    }

    public function index() {
        if(!$this->session->userdata('logged_in')) {
            redirect('/office/administrators/login');
        } else {
            redirect('/office/dashboard/');
        } 
    }

    public function login() {

        if($this->session->userdata('logged_in')) {
            redirect('/office/dashboard/');
        } 

        if($_SERVER['REQUEST_METHOD'] == 'POST') {

             $a = new Administrator();

            if($a->login($this->input->post('email'),$this->input->post('password'))) {
                    redirect('/office/dashboard/');
            } else {
                $this->messages->add('Unable to authenticate you', 'error');

            }
        }

        $data['error_messages'] = $this->messages->get('error'); 
        $this->load->view('/office/administrators/login', $data);
    }

    public function logout() {
        $this->simpleloginsecure->logout();
        redirect('office/administrators/login');
    }

}

?>
\$\endgroup\$
2
\$\begingroup\$

First Joseph's post. I agree with it mostly, but the first comment I'd like to clarify. A PHP opening tag should definitely be the first item on the page to avoid headers being sent prematurely, but the opening tag <?php or <? does not make a difference and depends upon your server. The traditional, long, PHP opener is usually the accepted norm because not all servers accept the later and it can cause compatibility issues. Such as if you were to include XML in your PHP then doing <?xml might cause issues. However, most all servers have a setting that allows you to enable it "short_open_tag". People still use the shorter form, but it does not have any performance differences. I'm sure this is what he meant, but originally reading it I thought he was saying that switching to the short form did that. So there's clarification.

Only other thing I want to point out is the following comment: "you can use the input class's post() to check for the post request". I'm not sure about CI, but unless it does the same REQUEST_METHOD check in its interior, then what you have there currently is fine. I just had a similar discussion on SO about checking the $_POST vs checking the REQUEST_METHOD and it is much better to check the request method. Checking the post array will work, but only because it is a hack. From the looks of this CI statement it is a similar thing. But I'd suggest looking into it for clarification.

Now, here are my suggestions:

Don't override a parent method if you aren't going to extend it. The child class inherits it automagically. The only reason to call the constructor again, or any inherited method for that matter, is if you were going to change something, extend it, before or after the parent method. Example, say you wanted to change a property $newProperty before it was used in the parent constructor (constructor is a bad example think a normal method). Then you can set that property just before calling it so that the parent method can use that new value. Or say that your parent method has a local variable $newVar and you want to use it in your new class. Then you can save that variable as a property or do something with it immediately in local scope. In short, just delete your constructor it is unnecessary as is.

public function __construct() {
    $this->newProperty = 'jkl';//can be used in parent method
    parent::__construct();
    echo $newVar;//came from parent method
    $this->newVar = $newVar;
}

Give your program some defaults instead of using if/else statements and/or calling methods multiple times. Doing this makes it easier to change functionality should you desire it. Say you didn't want to use redirect anymore, but maybe view instead, you'd have to change each occurrence of it. There's only two here, but if you had more it could be a pain.

public function index() {
    $redirect = '/office/dashboard/';

    if(!$this->session->userdata('logged_in')) {
        $redirect = '/office/administrators/login';
    }

    redirect( $redirect );
}
\$\endgroup\$
1
\$\begingroup\$

Here's modified code with comments:

<?php
//should be the very first characters

class Administrators extends CI_Controller {

    public function __construct() {
        parent::__construct();
    }

    //factor out the login check to isolate the common logic
    //also, for understandability, boolean checks should be prefixed
    //with "is" or "has" like phrases 
    //like "is it like this?", "[something] has this?"
    private function isLoggedIn(){
       return $this->session->userdata('logged_in');
    }

    public function index() {

        //use our new "isLoggedIn()"
        if(isLoggedIn()) {
            redirect('/office/dashboard/');
        } else {
            redirect('/office/administrators/login');
        } 
    }

    public function login() {

        if(isLoggedIn()) {
            redirect('/office/dashboard/');
        } 

        //you can use the input class's post() to check for the post request
        if($this->input->post()) {

            //for readability, it's more appropriate to
            //assign these values to verbose variables names...
            $administrator = new Administrator();
            $username = $this->input->post('email');
            $password = $this->input->post('password');

            //...so we know what arguments were sent
            if($administrator->login($username,$password)) {
                redirect('/office/dashboard/');
            } else {
                $this->messages->add('Unable to authenticate you', 'error');
            }
        }

        $data['error_messages'] = $this->messages->get('error'); 
        $this->load->view('/office/administrators/login', $data);
    }

    public function logout() {
        $this->simpleloginsecure->logout();
        redirect('office/administrators/login');
    }

}

//remove the closing ?>.
\$\endgroup\$

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.