Evaluating My Code

Question

I'm just trying to see if there's a better way to write the code i between the aestrick characters.

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

class Activate extends CI_Controller { 

public function __construct()
{
    parent::__construct();
    $this->load->model('users/usersmodel');
}

public function index()
{
    //Config Defaults Start
    $msgBoxMsgs = array();//msgType = dl, info, warn, note, msg
    $cssPageAddons = '';//If you have extra CSS for this view append it here
    $jsPageAddons =  '';//If you have extra JS for this view append it here
    $metaAddons = '';//Sometimes there is a need for additional Meta Data such in the case of Facebook addon's
    $siteTitle = '';//alter only if you need something other than the default for this view.
    //Config Defaults Start


    //examples of how to use the message box system (css not included).
    //$msgBoxMsgs[] = array('msgType' => 'dl', 'theMsg' => 'This is a Blank Message Box...');

    /**********************************************************Your Coding Logic Here, Start*/

    $userID     = $this->uri->segment(2);
    $registrationKey    = $this->uri->segment(3);

    if ( ((!is_numeric($userID)) || (is_null($userID))) || ((is_null($registrationKey)) || (!preg_match('/^[A-Za-z0-9]+$/', $registrationKey))) ) 
    {
        $bodyContent = $this->config->item('defaultTemplate') ."/404";//which view file  
    } 
    else 
    {
        $registrationDetails = $this->usersmodel->getRegistrationDetails($userID);
        if (!is_null($registrationDetails))
        {
            if ($this->usersmodel->activateUser($userID, $registrationKey))
            {
                $jsPageAddons .= '<script src='.base_url().'assets/'. $this->config->item('defaultTemplate') .'/js/validate/activate.js></script>';
                $message = 'User was activated successfully! You may now login!';   
                $bodyContent = $this->config->item('defaultTemplate') ."/usermanagement/forms/activate";//which view file      
            }
            else
            {
                $message = 'User was not activated successfully! Please try again with the right credentials!';  
                $bodyContent = $this->config->item('defaultTemplate') ."/usermanagement/forms/activate";//which view file  

            }
            $this->data['message'] = $message;                                
        }
        else
        {
            $bodyContent = $this->config->item('defaultTemplate') ."/404";  
        }
    }

    $bodyType = "full";//type of template   

    /***********************************************************Your Coding Logic Here, End*/

    //Double checks if any default variables have been changed, Start.
    //If msgBoxMsgs array has anything in it, if so displays it in view, else does nothing.      
    if(count($msgBoxMsgs) !== 0)
    {
        $msgBoxes = $this->msgboxes->buildMsgBoxesOutput(array('display' => 'show', 'msgs' =>$msgBoxMsgs));
    }
    else
    {
        $msgBoxes = array('display' => 'none');
    }

    if($siteTitle == '')
    {
        $siteTitle = $this->metatags->SiteTitle(); //reads 
    }

    //Double checks if any default variables have been changed, End.

    $this->data['msgBoxes'] = $msgBoxes;
    $this->data['cssPageAddons'] = $cssPageAddons;//if there is any additional CSS to add from above Variable this will send it to the view.
    $this->data['jsPageAddons'] = $jsPageAddons;//if there is any addictional JS to add from the above variable this will send it to the view.
    $this->data['siteTitle'] = $siteTitle;//defaults can be changed via models/metatags.php
    $this->data['bodyType'] = $bodyType;
    $this->data['bodyContent'] = $bodyContent;
    $this->load->view($this->config->item('defaultTemplate') .'/usermanagement/index', $this->data);
}

}

/* End of file activate.php */ 
/* Location: ./application/controllers/activate.php */

UPDATE:

Here's my update tell me what you think now?

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

class Activate extends CI_Controller { 

public function __construct()
{
    parent::__construct();
    $this->load->model('users/usersmodel');
}

public function index()
{
    //Config Defaults Start
    $msgBoxMsgs = array();//msgType = dl, info, warn, note, msg
    $cssPageAddons = '';//If you have extra CSS for this view append it here
    $jsPageAddons =  '';//If you have extra JS for this view append it here
    $metaAddons = '';//Sometimes there is a need for additional Meta Data such in the case of Facebook addon's
    $siteTitle = '';//alter only if you need something other than the default for this view.
    //Config Defaults Start


    //examples of how to use the message box system (css not included).
    //$msgBoxMsgs[] = array('msgType' => 'dl', 'theMsg' => 'This is a Blank Message Box...');

    /**********************************************************Your Coding Logic Here, Start*/

    $userID = $this->uri->segment(2);
    $registrationKey = $this->uri->segment(3);

    if( ! $this->verify( $userID, $registrationKey ) )
    {
        $bodyContent = $this->kow->return404();
    } 
    else 
    {
        if ( ! isRegistered( $userID ))
        {
            $message = 'User was not registered successfully! Please try again with the right credentials!'; 

            if ($this->usersmodel->activateUser($userID, $registrationKey))
            {
                $jsPageAddons .= '<script src='.base_url().'assets/'. $this->config->item('defaultTemplate') .'/js/validate/activate.js></script>';
                $message = 'User was activated successfully! You may now login!';        
            }

            $bodyContent = $this->config->item('defaultTemplate') ."/usermanagement/forms/activate";//which view file  
            $this->data['message'] = $message;                                
        }
        else
        {
            $bodyContent = $this->kow->return404();
        }
    }

    $bodyType = "full";//type of template   

    /***********************************************************Your Coding Logic Here, End*/

    //Double checks if any default variables have been changed, Start.
    //If msgBoxMsgs array has anything in it, if so displays it in view, else does nothing.      
    if(count($msgBoxMsgs) !== 0)
    {
        $msgBoxes = $this->msgboxes->buildMsgBoxesOutput(array('display' => 'show', 'msgs' =>$msgBoxMsgs));
    }
    else
    {
        $msgBoxes = array('display' => 'none');
    }

    if($siteTitle == '')
    {
        $siteTitle = $this->metatags->SiteTitle(); //reads 
    }

    //Double checks if any default variables have been changed, End.

    $this->data['msgBoxes'] = $msgBoxes;
    $this->data['cssPageAddons'] = $cssPageAddons;//if there is any additional CSS to add from above Variable this will send it to the view.
    $this->data['jsPageAddons'] = $jsPageAddons;//if there is any addictional JS to add from the above variable this will send it to the view.
    $this->data['siteTitle'] = $siteTitle;//defaults can be changed via models/metatags.php
    $this->data['bodyType'] = $bodyType;
    $this->data['bodyContent'] = $bodyContent;
    $this->load->view($this->config->item('defaultTemplate') .'/usermanagement/index', $this->data);
}

private function verify( $userID, $registrationKey ) 
{

    if( ! $userID || ! is_numeric( $userID ) ) 
    { 
        return false; 
    }

    $matches = preg_match('/^[A-Za-z0-9]+$/', $registrationKey);//You should comment this to explain what it does

    if ( ! $registrationKey || ! $matches ) 
    { 
        return false; 
    }

    return TRUE;
}

}

/* End of file activate.php */ 
/* Location: ./application/controllers/activate.php */

FINAL UPDATE

Hopefully. Please do one final look over for me please.

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

class Activate extends CI_Controller { 

public function __construct()
{
    parent::__construct();
    $this->load->model('users/usersmodel');
}

public function index()
{
    //Config Defaults Start
    $msgBoxMsgs = array();//msgType = dl, info, warn, note, msg
    $cssPageAddons = '';//If you have extra CSS for this view append it here
    $jsPageAddons =  '';//If you have extra JS for this view append it here
    $metaAddons = '';//Sometimes there is a need for additional Meta Data such in the case of Facebook addon's
    $siteTitle = '';//alter only if you need something other than the default for this view.
    $message = '';
    //Config Defaults Start


    //examples of how to use the message box system (css not included).
    //$msgBoxMsgs[] = array('msgType' => 'dl', 'theMsg' => 'This is a Blank Message Box...');

    /**********************************************************Your Coding Logic Here, Start*/

    $userID = $this->uri->segment(2);
    $registrationKey = $this->uri->segment(3);

    $bodyContent = $this->kow->return404();

    if( $this->verify( $userID, $registrationKey ) ) 
    {
        if ( ! isRegistered( $userID ))
        {
            $message = 'User was not registered successfully! Please try again with the right credentials!'; 

            if ($this->auth->activateUser($userID, $registrationKey))
            {
                $jsPageAddons .= '<script src='.base_url().'assets/'. $this->config->item('defaultTemplate') .'/js/validate/activate.js></script>';
                $message = 'User was activated successfully! You may now login!';        
            }

            $bodyContent = $this->config->item('defaultTemplate') ."/usermanagement/forms/activate";//which view file                           
        }
    }

    $bodyType = "full";//type of template   

    /***********************************************************Your Coding Logic Here, End*/

    //Double checks if any default variables have been changed, Start.
    //If msgBoxMsgs array has anything in it, if so displays it in view, else does nothing.      
    if(count($msgBoxMsgs) !== 0)
    {
        $msgBoxes = $this->msgboxes->buildMsgBoxesOutput(array('display' => 'show', 'msgs' =>$msgBoxMsgs));
    }
    else
    {
        $msgBoxes = array('display' => 'none');
    }

    if($siteTitle == '')
    {
        $siteTitle = $this->metatags->SiteTitle(); //reads 
    }

    //Double checks if any default variables have been changed, End.
    $this->data['message'] = $message;      
    $this->data['msgBoxes'] = $msgBoxes;
    $this->data['cssPageAddons'] = $cssPageAddons;//if there is any additional CSS to add from above Variable this will send it to the view.
    $this->data['jsPageAddons'] = $jsPageAddons;//if there is any addictional JS to add from the above variable this will send it to the view.
    $this->data['siteTitle'] = $siteTitle;//defaults can be changed via models/metatags.php
    $this->data['bodyType'] = $bodyType;
    $this->data['bodyContent'] = $bodyContent;
    $this->load->view($this->config->item('defaultTemplate') .'/usermanagement/index', $this->data);
}

private function verify( $userID, $registrationKey ) 
{

    if( ! $userID || ! is_numeric( $userID ) ) 
    { 
        return false; 
    }

    $matches = preg_match('/^[A-Za-z0-9]+$/', $registrationKey);//You should comment this to explain what it does

    if ( ! $registrationKey || ! $matches ) 
    { 
        return false; 
    }

    return true;
}

}

/* End of file activate.php */ 
/* Location: ./application/controllers/activate.php */

mseancole · Accepted Answer · 2012-06-05 13:31:55Z

I know you are specifically asking about just the code between the commented section, but I feel the rest deserves some attention as well. I will try to focus primarily upon what you requested though.

Separating Your Code

First off, there is no reason for all that code to be contained in a single method. Separate your code into smaller pieces so that they perform single functions. This improves legibility, is helpful for debugging, and makes your code more reusable, among other things. As I go through I'll try and point out a few examples.

Verifying Your Variables

There's a few things I would change about how you are currently verifying your variables. First, you should separate the processes, not just by moving it to a new method, but you should also separate it by variable. Right now you are checking for both $userID and $registrationKey together. There's nothing wrong with it except that it is a little hard to read and does not allow for custom error support should you decide to do so at a future date. Second, assuming you are not using 0 as a value for either of those two variables, you can just check each as a boolean (! $var) instead of using is_null(). This way you can perform the same function on values other than numeric or null without needing to write additional conditions. Checking it as a boolean will return FALSE if its values is empty, FALSE, NULL, 0, or a string version of any of those. The final way in which I would change this, is by moving your preg_match() statement outside of the if statement. How you have it now strongly contributes to its illegibility. These points also hold true for $registrationDetails.

private function _verify( $userID, $registrationKey ) {
    if( ! $userID || ! is_numeric( $userID ) ) { return FALSE; }

    $matches = preg_match('/^[A-Za-z0-9]+$/', $registrationKey);//You should comment this to explain what it does
    if ( ! $registrationKey || ! $matches ) { return FALSE; }

    return TRUE;
}

//Usage
if( ! $this->_verify( $userID, $registrationKey ) ) { $bodyContent = $this->config->item( 'defaultTemplate' ) . '/404'; }

Take a look at PHP's filter_var() function (PHP >= 5.2) as a better way of verifying your variables. I didn't mention it until now so that I could comment on the above as it can be adapted elsewhere in your code, not necessarily here, but maybe in your other code. With filter_var() you can just use the desired flag(s) to specify the type of data you want a variable to contain and it will verify it and/or sanitize it or return FALSE if it does not meet its criteria. Also, I'm not sure what your regex statement does, but I'm sure this function can do something similar for you.

if( ! filter_var( $userID, FILTER_VALIDATE_INT ) ) { //etc... }

Reusing Code

This is a simple instance of it, but instead of continuously writing out the bit for the "404" page, I would make it into a function to return that value.

private function _return404() { return $this->config->item( 'defaultTemplate' ) . '/404'; }

Here's a better example. When you are verifying your $registrationDetails. Your $bodyContent remains the same unless $registrationDetails is null, the only thing that changes is your message, so I would change it to something more like...

    if ( ! $registrationDetails) {
        $message = 'User was not activated successfully! Please try again with the right credentials!';//Message will change on success

        if ($this->usersmodel->activateUser($userID, $registrationKey))
        {
            $jsPageAddons .= '<script src='.base_url().'assets/'. $this->config->item('defaultTemplate') .'/js/validate/activate.js></script>';
            $message = 'User was activated successfully! You may now login!';   
        }

        $bodyContent = $this->config->item('defaultTemplate') ."/usermanagement/forms/activate";//which view file      
        $this->data['message'] = $message;                                
    }

Also, you never use $registrationDetails except as a check. If that is its only purpose, you should create a method in your user model called isRegistered() and have it return a boolean so that you can use it for that purpose. No need to load what I'm assuming is an array of credentials just to verify a user has been registered.

jsPageAddons

Assuming that only the file itself, and maybe the last directory changes, I would convert $jsPageAddons to accept an array of filenames and pass it to a method to return the appropriate string. This way you do not have to rewrite each script tag. This also holds for $cssPageAddons.

private function _setScript( $file ) { return '<script src="' . base_url() . "assets/{$this->config->item('defaultTemplate')}/js/$file.js\"></script>"; }
//Usage
$scripts = '';
foreach( $jsPageAddons AS $file ) { $scripts .= $this->_setScript( $file ); }//$file = 'validate/activate';

Or, if you were to do it in the view:

//set in controller $jsPath = base_url() . "assets/{$this->config->item('defaultTemplate')}/js/";
<?php foreach( $jsPageAddons AS $file ) : ?>

<script src="<?php echo $jsPath . $file; ?>"></script>

<?php endforeach; ?>

Site Variables

Instead of typing out each of your site variables like so:

$this->data['msgBoxes'] = $msgBoxes;

It would be smarter to do it like so:

$this->data = compact(
    'msgBoxes',
    'cssPageAddons',
    'jsPageAddons',
    'siteTitle',
    'bodyType',
    'bodyContent'
);//$msgBoxes, $cssPageAddons, $jsPageAddons, $siteTitle, $bodyType, $bodyContent

I added the comment at the end so that your IDE can find those variables later if needed as compact() is not very IDE friendly.

Minor Things

You have excessive parenthesis. Half of them aren't necessary and its already challenging to tier your braces, brackets, and parenthesis without adding extra :)

if ( (!is_numeric($userID) || is_null($userID)) || (is_null($registrationKey) || !preg_match('/^[A-Za-z0-9]+$/', $registrationKey)) )

Unless you are specifically looking for "0" do not use equality operators on integers, use the greater than > or less than < operators.

if(count($msgBoxMsgs) > 0) { //etc...

This is preference really, but I hardly ever see is_null() anymore. Its still supported, and probably always will be. But more often I see an equality comparison === NULL. In the documentation it is show to be faster as well, but that doesn't matter unless you are an optimization nut.

You'll probably want to perform the same function on $siteTitle whether it is empty, FALSE, NULL, or 0. So this will be another one of those instances where I'd just perform a boolean comparison on it.

if( ! $siteTitle ) { //etc...

Final Thoughts

braunbaer had some good advice as well.

Now I'm assuming that the code that you highlighted is your own and the code wrapping it is part of a framework. Otherwise, why do you have all these variables that you never use? The only one I see you use is $jsPageAddons, and then $siteTitle is checked even though nothing was done to it before being set by an outside method. The rest are always blank. If it is necessary for your rendering method to have all these fields to work, you might want to take a look at it too. Perform isset() checks on those values you have not set and only set those you need, when you need them. Processing all these other variables is just dead weight. Admittedly light dead weight, but dead weight none-the-less. And confusing. If these are only here as a guideline of what variables to use and their types, I would document it in the PHPDOC comments instead.

Also, you have a 0% acceptance ratio. I don't know if this is because you are new and SO starts you off at 0 (don't think so) or if you just don't give credit to those who answer your questions. You aren't likely to get much help if you don't go back and accept some of those answers. I like helping which is why I answered your question, but some others might not feel the same and will ignore your questions because of it. Just a friendly pointer :) Good luck!

UPDATE

The changes that you made look fine. I am a little confused as to where isRegistered() is defined so that it can be called like a normal function instead of a method though. Assuming this was an oversight.

Just had a thought on how to improve it even more. No need to set your $bodyContent so many times. It directs to a 404 page if there are any issues, so you only need to worry about changing it if there is success. Just set it up initially to direct to the 404. This means that the return404() method I suggested is now unnecessary, unless you use it elsewhere.

$bodyContent = $this->kow->return404();

if( $this->verify( $userID, $registrationKey ) ) {
    if ( ! isRegistered( $userID ))
    {
        $message = 'User was not registered successfully! Please try again with the right credentials!'; 

        if ($this->usersmodel->activateUser($userID, $registrationKey))
        {
            $jsPageAddons .= '<script src='.base_url().'assets/'. $this->config->item('defaultTemplate') .'/js/validate/activate.js></script>';
            $message = 'User was activated successfully! You may now login!';        
        }

        $bodyContent = $this->config->item('defaultTemplate') ."/usermanagement/forms/activate";//which view file  
        $this->data['message'] = $message;                                
    }
}

One last thing I noticed. You are not setting $data['message'] with the rest of the $data array. I'm assuming it is because $message is not always set. If I were you, I would create an empty $message variable in with the "Config Defaults" and move that $data array definition with the rest to keep your coding logic together.

I've never seen compact() before, thanks for adding it to your answer. — Baylor Rae', Jun 4 '12 at 21:58
This is really awesome however what do you mean "Config Defaults"? — Jeff Davidson, Jun 5 '12 at 16:26
You have them wrapped between two comments (//Config Defaults Start) at the start of your index() method... — mseancole, Jun 5 '12 at 16:40

braunbaer · Answer 2 · 2012-06-04 18:49:47Z

i think a good way to improve the code would be using exceptions and extracting some stuff into reusable code.

you could change

$userID     = $this->uri->segment(2);

to

$userId = $this->getUserIdByUri($this->uri)

this way you have a single point of change if the user id should ever move from segment 2 to 3 r any other segment or event out of the uri. (same for the registrationKey)

if ( ((!is_numeric($userID)) || (is_null($userID))) || ((is_null($registrationKey)) || (!preg_match('/^[A-Za-z0-9]+$/', $registrationKey))) )

thats the point where i would start my try/catch block and extract the validation into another method like

try {
  $this->checkUserId($userID)
} catch (InvalidUserIdException) {
  $bodyContent = $this->config->item('defaultTemplate') ."/404";
}

same thing for $registrationDetails just export the check into a checkRegistrationDetails method and throw an exception.

one more thing would be to extract some "magic strings / values" into const values like

const TEMPLATE_DEFAULT = 'defaultTemplate';
const TEMPLATE_FORM_ACTIVE = '/usermanagement/forms/activate';

but i think thats a thing of taste. so after my changes the code would look like

/**********************************************************Your Coding Logic Here, Start*/

    $userID     = $this->getUserIdByUri($this->uri);
    $registrationKey    = $this->getRegKeyByUri($this->uri);

    try {
        $this->checkUserId($userID); // throw InvalidUserIdException

        $registrationDetails = $this->usersmodel->getRegistrationDetails($userID);
        $this->checkRegistrationDetails($registrationDetails); // throws InvalidRegDetails

        if ($this->usersmodel->activateUser($userID, $registrationKey))
        {
            $jsPageAddons .= $this->getActivJS();
            $message = 'User was activated successfully! You may now login!';
            $bodyContent = $this->config->item(self::TEMPLATE_DEFAULT) . self::TEMPLATE_FORM_ACTIVATE;//which view file
        }
        else
        {
            $message = 'User was not activated successfully! Please try again with the right credentials!';
            $bodyContent = $this->config->item(self::TEMPLATE_DEFAULT) . self::TEMPLATE_FORM_ACTIVATE;//which view file

        }
        $this->data['message'] = $message;

    } catch (InvalidUserIdException $e) {
        $bodyContent = $this->config->item('defaultTemplate') ."/404";
    } catch (InvalidRegDetails $e) {
        $bodyContent = $this->config->item('defaultTemplate') ."/404";
    } catch (Exception $e) {
        // something unexpected happened (- ;
    }


    $bodyType = "full";//type of template

    /***********************************************************Your Coding Logic Here, End*/

maybe i could help you a little. These things i pointed out are not any rules you HAVE to follow. Just some advice (- :

can you better explain the top part of your answer with factoring in the uri segments. — Jeff Davidson, Jun 5 '12 at 8:43
the uri segment is just another seperation of code. For now you have hardcoded that your id is in segment 3 which also is a "magic number". Also what is "$this->kow->" in your update? typo? — braunbaer, Jun 5 '12 at 8:52
kow is my library that is different functions I use across my scripts — Jeff Davidson, Jun 5 '12 at 9:02

asked	1 year ago
viewed	137 times
active	1 year ago

current community

your communities

more stack exchange communities

Evaluating My Code

2 Answers

Your Answer

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

Hot Network Questions

current community

your communities

more stack exchange communities

Evaluating My Code

2 Answers

Your Answer

Sign up or log in

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