1
\$\begingroup\$

I used this code on my own site and now trying to transfer it to my new site which uses codeigniter. I'm not sure how I can eliminate some of this code and still maintain its purpose and functionality with merging it into my controller, model, and library.

<div id="headerCharacter">   

                <?php
        if ($access_level_id == 2 || $access_level_id == 3) {
            $query = "SELECT
                    characters.id
                FROM
                    characters"; 
            $result = mysqli_query ($dbc,$query); 
            $total_num_characters = mysqli_num_rows($result);

            $query = "SELECT
                    user_characters.id
                FROM
                    user_characters
                INNER JOIN user_accounts
                    ON user_accounts.id = user_characters.user_id"; 
        } else {    
            $query = "SELECT
                    user_characters.id
                FROM
                    user_characters
                INNER JOIN user_accounts
                    ON user_accounts.id = user_characters.user_id
                WHERE
                    user_accounts.id = '".$user_id."'"; 
        }
        $result = mysqli_query($dbc,$query);

        $num_available_characters = mysqli_num_rows($result);
        if (($num_available_characters > "1") || (($access_level_id == 2 || $access_level_id == 3) && (isset($total_num_characters)) && ($total_num_characters > "0"))) {
        ?>
            <form method="POST" id="change_default_character">
            <select class="dropdown" name="new_default_character_id" id="new_default_character_id" title="Select Character">                
        <?php

            if ($default_character_id > "0") {
                print "<option value=".$default_character_id.">".$default_character_name;
            } else {
                print "<option value=0>- Select -";
            }
            if ($access_level_id == 2 || $access_level_id == 3) {
                $query = "SELECT
                        characters.id,
                        characters.character_name
                    FROM
                        characters
                    WHERE
                        characters.id <> '".$default_character_id."' AND
                        characters.status_id = '1'
                    ORDER BY
                        characters.character_name"; 
            } else {
                $query = "SELECT
                        characters.id,
                        characters.character_name
                    FROM
                        characters
                    INNER JOIN
                        user_characters
                        ON characters.id = user_characters.character_id                     
                    INNER JOIN
                        user_accounts
                        ON user_accounts.id = user_characters.user_id
                    WHERE
                        user_accounts.id = '".$user_id."' AND
                        user_characters.character_id <> '".$default_character_id."' AND
                        characters.status_id = '1'
                    ORDER BY
                        characters.character_name"; 
            }
            $result = mysqli_query ($dbc,$query);
            $num_rows = mysqli_num_rows ($result);
            if ($num_rows > 0) {
                if ($access_level_id == 2 || $access_level_id == 3) {
                    print "<optgroup label=\"** Active Characters **\">";
                }
                while ( $row = mysqli_fetch_array ( $result, MYSQL_ASSOC ) ) {
                    print "<option value=\"".$row['id']."\">".$row['character_name']."</option>\r";
                }
            }
            if ($access_level_id == 2 || $access_level_id == 3) {
                $query = "SELECT
                        characters.id,
                        characters.character_name
                    FROM
                        characters
                    WHERE
                        characters.id <> '".$default_character_id."' AND
                        characters.status_id = '2'
                    ORDER BY
                        characters.character_name"; 
            } else {
                $query = "SELECT
                        characters.id,
                        characters.character_name
                    FROM
                        characters
                    LEFT JOIN
                        user_characters
                        ON characters.id = user_characters.character_id                     
                    LEFT JOIN
                        user_accounts
                        ON user_accounts.id = user_characters.user_id
                    WHERE
                        user_accounts.id = '".$user_id."' AND
                        user_characters.character_id <> '".$default_character_id."' AND
                        characters.status_id = '2'
                    ORDER BY
                        characters.character_name"; 
            }
            $result = mysqli_query ($dbc,$query); 
            $num_nows = mysqli_num_rows($result);
            if ($num_rows > "0") {
                print "<optgroup label=\"** Inactive Characters **\">"; 
                while ( $row = mysqli_fetch_array ( $result, MYSQL_ASSOC ) ) {
                    print "<option value=\"".$row['id']."\">".$row['character_name']."</option>\r";    
                }
            }
            ?>
            </select>
            </form> 
        <?php
        } else {
            print "<h1>".$default_character_name."</h1>\n";
        }
        ?>
        </div>

EDIT :

If user role is a user or editor then either display their default character or if more than one character then display dropdown of all handled characters. If user role is an administrator or webmaster then display their default character or ALL characters.

Displays Active (status-1), Inactive(status-2), Injured(status-3, Alumni(status-4) in separate option groups

Controller:

$this->data['userRoster'] = $this->kowauth->getRosterList($this->data['userData']->usersRolesID);

Library:

/**
 * Get roster list
 *
 * @param   integer
 * @return  object/NULL
 */
function getRosterList($usersRolesID)
{
if (($usersRolesID == 4) || ($usersRolesID == 5))
{
    return $this->ci->users->getAllRoster();
} 
else
{
    return $this->ci->users->getRosterByUserID($this->ci->session->userdata('usersID'));
}
}

Model:

/**
* Get roster list
*
* @return  object/NULL
*/
function getAllRoster()
{       
$this->db->select('rosterName');
$this->db->from('rosterList');
$this->db->order_by('rosterName');
$query = $this->db->get();
if ($query->num_rows() > 0) 
{
    return $query->result();
}
return null;
}

/**
* Get list of roster by user ID
*
* @return  object/NULL
*/
function getRosterByUserID($usersID)
{
$this->db->select('rosterName');
$this->db->from('rosterList');
$this->db->where('userID', $usersID);
$this->db->order_by('rosterName');
$query = $this->db->get();
if ($query->num_rows() > 0) 
{
    return $query->result();
}
return null;
}
\$\endgroup\$
1
\$\begingroup\$

First, you should really use prepared statements instead of stuff like user_accounts.id = '".$user_id."'";: this will avoid security issues. Let's look at your actual question now.

I'd advise you to go through the CodeIgniter officiel tutorial which will help you to understand how Models, Views and Controllers relate to each other. You should remember that they only exist to help you to organise your code in order to avoid a huge mess full of inline SQL queries and HTML code which can get out of control easily.

This means you can start by putting your code in a controller function, and refactor iteratively to get a nice, easy to understand code after a few steps. A few rules of thumbs to know where to put your code:

  • Conditional logic (eg. if ($access_level_id == 2 || $access_level_id == 3) in your code should stay in the controller. This what tells the application "do this only in this case, and that in this other case".
  • SQL queries should all go in the model. Your queries could go in one single method if they mean the same thing. Parameters to this method will help you differentiate the actual SQL queries.
  • HTML code should only be produced in the view. The view should only display content, not retrieve data or use logic.

To wrap it up, this means that the first code to get executed is in the controller. Depending on the user session and parameters, it could need to modify the database (eg. add a new user). It should ask the model to do this. It can also ask the model for some other unrelated data, and pass it up to the view which is going to display it.

\$\endgroup\$
  • \$\begingroup\$ I've updated my post! \$\endgroup\$ – Jeff Davidson Apr 5 '12 at 19:03
  • \$\begingroup\$ Yep, you got it. Make sure to use === in your library to avoid '5things' to work ('5things' == 5 is true, but '5' === 5 is false in PHP). \$\endgroup\$ – Quentin Pradet Apr 6 '12 at 11:58

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.