Tips for coding style required

Coding Critique is the place to post source code for peer review by other members of DevNetwork. Any kind of code can be posted. Code posted does not have to be limited to PHP. All members are invited to contribute constructive criticism with the goal of improving the code. Posted code should include some background information about it and what areas you specifically would like help with.

Popular code excerpts may be moved to "Code Snippets" by the moderators.

Moderator: General Moderators

Post Reply
User avatar
spamyboy
Forum Contributor
Posts: 266
Joined: Sun Nov 06, 2005 11:29 am
Location: Lithuania, vilnius

Tips for coding style required

Post by spamyboy »

Code: Select all

 
if(!defined('GCMS')){die('access denied');}
 
// Loading user class
require_once('user.class.php');
$user   = new user_class($db);
 
// Module control
$task   = (!empty($_GET['task'])) ? $_GET['task'] : 'user';
 
// Module vars
$smarty->assign('module_id',    'user');
$smarty->assign('module_title', $l['users']);
 
// Module directory
define('MODULE_DIR', 'module/backend/user/');
 
// Module menu
$module_menu    = array(
    array('task'=>'user',       'menu_link'=>'admin.php?module=user',               'menu_title'=>$l['users']),
    array('task'=>'add-user',   'menu_link'=>'admin.php?module=user&task=add-user', 'menu_title'=>$l['add_new_user'])
);
 
// Removing current page from module menu
$module_menu    = $framework->remove_from_array_by_value($module_menu, $task, 'task');
$smarty->assign('module_menu', $module_menu);
 
switch($task){
    // Used to display users
    case 'user':
        // Module template
        $smarty->assign('base_cotent_tpl', MODULE_DIR.'template/admin.user.map.tpl');
        
        // Lauding info about all users
        $result = $db->query("SELECT `user_id`, `user_fullname` FROM `gcms_user` ORDER BY `user_id` DESC;");
        $result = $result->fetchAll();
        $smarty->assign('user', $result);
    break;
    
    // Used to delete user
    case 'delete-user':
        if (!empty($_GET['id']) and !is_numeric($_GET['id'])) {
            $framework->redirect('admin.php?module=user', $l['id_not_given_or_misunderstood'], 'error');
        } else if (!empty($_GET['id']) and !$user->check_if_value_exists('user_id', $_GET['id'])) {
            $framework->redirect('admin.php?module=user', $l['user_not_found'], 'error');
        } else if (!empty($_GET['id'])) {
            $user->delete_user($_GET['id']);
            $framework->redirect('admin.php?module=user', $l['user_successfully_removed'], 'message');
        }
    break;
    
    // Used to add user
    case 'add-user':
        // Module subtitle
        $smarty->assign('module_subtitle',      $l['add_new_user']);
        // Module template
        $smarty->assign('base_cotent_tpl', MODULE_DIR.'template/admin.user.form.tpl');
        
        // Handling add user form
        if (!empty($_POST)) {
            if (empty($_POST['user_fullname']) or empty($_POST['user_username']) or empty($_POST['user_password']) or empty($_POST['user_password_confirm'])) {
                $framework->redirect('admin.php?module=user&task=add-user', $l['red_star_marked_fields_are_required'], 'error');
            } else if ($user->check_if_value_exists('user_username', $_POST['user_username'])) {
                $framework->redirect('admin.php?module=user&task=add-user', $l['username_already_taken'], 'error');
            } else if ($_POST['user_password']!=$_POST['user_password_confirm']) {
                $framework->redirect('admin.php?module=user&task=add-user', $l['passwords_do_not_match'], 'error');
            } else {
                // Creating new user
                $user_id    = $user->create_new_user($_POST['user_username'], $_POST['user_fullname'], $auth->secure($_POST['user_password']), $_POST['user_note']);            
                $framework->redirect('admin.php?module=user&task=edit-user&id='.$user_id, $l['user_successfully_created'], 'message');
            }
        }
        
        // Loading empty values to prevent errors
        $smarty->assign('user', NULL);
    break;
    
    // User to edit users
    case 'edit-user':
        // Module subtitle
        $smarty->assign('module_subtitle',  $l['edit_user']);
        // Module template
        $smarty->assign('base_cotent_tpl', MODULE_DIR.'template/admin.user.form.tpl');
        
        // Checking if #id is given and if it is numeric
        if (empty($_GET['id']) or !is_numeric($_GET['id'])) {
            $framework->redirect('admin.php?module=user', $l['id_not_given_or_misunderstood'], 'error');
        }
        
        // Loading info about user
        if ($user->check_if_value_exists('user_id', $_GET['id'])) {
            $user_result    = $user->get_user($_GET['id'], array('user_id', 'user_username', 'user_email', 'user_email', 'user_fullname', 'user_note'));
            $smarty->assign('user', $user_result);
        } else {
            $framework->redirect('admin.php?module=user', $l['user_not_found'], 'error');
        }
        
        // Handling user update form
        if (!empty($_POST)) {
            // Checking if there aren't any required field left empty
            if (empty($_POST['user_id']) or empty($_POST['user_username']) or empty($_POST['user_fullname'])) {
                $framework->redirect($_SERVER['HTTP_REFERER'], $l['red_star_marked_fields_are_required'], 'error');
            }
            
            // Handling users password update
            if (!empty($_POST['user_password'])) {
                // Check if entered password matches current users password.
                $result = $user->check_users_password($_POST['user_id'], $auth->secure($_POST['user_password']));
                
                if ($result==FALSE) {
                    $framework->redirect($_SERVER['HTTP_REFERER'], $l['wrong_password'], 'error');
                } else if (empty($_POST['user_new_password']) or empty($_POST['user_new_password_confirm'])) {
                    $framework->redirect($_SERVER['HTTP_REFERER'], $l['new_password_not_entered'], 'error');
                } else if ($_POST['user_new_password']!=$_POST['user_new_password_confirm']) {
                    $framework->redirect($_SERVER['HTTP_REFERER'], $l['new_passwords_do_not_match'], 'error');
                } else {
                    // Update users password
                    $user->update_user($_POST['user_id'], array('user_password'=>$auth->secure($_POST['user_new_password'])));
                }
            }
            
            // Handling users email update
            if (!empty($_POST['user_email']) and !$user->check_email_address($_POST['user_email'])) {
                $framework->redirect('admin.php?module=user&task=edit-user&id='.$_POST['user_id'], $l['wrong_email_format'], 'error');
            } else if (!empty($_POST['user_email'])) {
                $user->update_user($_POST['user_id'], array('user_email'=>$_POST['user_email']));
            }
            
            // Handling user information update
            if (!$user->check_if_value_exists('user_username', $_POST['user_username'])) {
                $user->update_user($_POST['user_id'], array('user_username'=>$_POST['user_username']));
            } else if ($user->check_if_value_exists('user_username', $_POST['user_username']) and ($user_result['user_username']!==$_POST['user_username'])) {
                $framework->redirect('admin.php?module=user&task=edit-user&id='.$_POST['user_id'], $l['username_already_taken'], 'error');
            }
            
            // Updating misc user information
            $user->update_user($_POST['user_id'], array('user_fullname'=>$_POST['user_fullname'], 'user_note' => $_POST['user_note']));
            $framework->redirect($_SERVER['HTTP_REFERER'], $l['user_information_successfully_updated'], 'message');
        }
    break;
}
 
I believe that I still do many irrational things and as everyone else I would like to reduce that as much as possible. So basically I am asking for tips and tricks that I can use in my coding. For example here, since it is most common tasks to proceed.
And tips and tricks are welcome.
User avatar
infolock
DevNet Resident
Posts: 1708
Joined: Wed Sep 25, 2002 7:47 pm

Re: Tips for coding style required

Post by infolock »

This probably the hardest part of any developer who is finally excelling: finding out where they are going wrong and how to fix it.

When you excel, you start seeing the whole "development" thing differently, and you want to make sure that the code you are writing isn't bogged down, isn't repetative, and that it is easy to follow/documented well.

To critique your code, first of all this is actually a simple smarty template action file. The way you laid it out is nicely done. I like the indentions, I like how the code flows and feels.

The only thing I could say is be more descriptive with your variables. Variable names are a 2nd pair of eyes bread and butter when it comes to trying to figure out what variable definitions are set to and how they were set without ever needing to see the definition itself. Most of the variables you have are nice, but a variable like $l doesn't tell me jack about what it does, and I have to search through the code to find out.

Also, with $result, you should be more specific. A $result variable should be defined once per query, and then if you do a fetch_assoc, call it $user_name_rows or $order_rows, etc. Renaming result variables to be set for use of rows is not explaining what that data is going to hold, nor does it tell the next developer what you are doing.

If you are going to use $result though, be sure to comment that fact very well. Which brings me to my final point:

Comment, and comment well. Don't over comment as no one wants to shift through lines and liens of commenting, but things like your switch statement should have a comment per CASE statement, which will indicate WHAT, WHY, and WHERE.

If you find code that doesn't seem very self-explanatory, comment it. It will not only help the next developer out, but it will save you time in the future.

Otherwise, looks like you are coming along quite nicely. Find some complex code you have though, and paste that as an example, and then we can really get down and dirty ;)
gutscheinco
Forum Newbie
Posts: 1
Joined: Sat Nov 08, 2008 12:03 pm

Re: Tips for coding style required

Post by gutscheinco »

thanks :D
jason.carter
Forum Commoner
Posts: 35
Joined: Sat Jan 10, 2009 10:05 am

Re: Tips for coding style required

Post by jason.carter »

Couple of things
1) A default case for your switch statement is always a good idea.
2) You use the $_SERVER['HTTP_REFERER'] which is not a great idea for redirecting to users on your own site. You know the domain name so you can simply use that OR just use redirect and page name like index.php etc and this work automatically on whatever domain.
Main issues of HTTP_REFERER
a) It can be spoofed.
b) Some browser security settings will not send this to the server. Which means this can cause the redirection not to work.
Post Reply