Experienced PHP developer, please REVIEW my code

Question

I was hoping someone with experience in PHP could review this code and suggest areas where it might be improved.

I've posted a snippet of the code as it's quite long, but I've also created a post on pastebin. That post can be viewed here.

Code snippet:

function school_page_handler($page){
global $CONFIG;

switch ($page[0])
{
    case 'admin':
        switch($page[1]) {
            case 'delete':
                set_input('method', 'delete');
                set_input('admin_guid', $page[2]);
                break;
            default:
                set_input('method','list');
        }
        include $CONFIG->pluginspath . 'school/pages/admin.php';
        break;

    case 'approval':
        switch($page[1]) {
            case 'event':
                set_input('method','event');
                break;
            case 'product':
                set_input('method','product');
                break;
            case 'announcement':
                set_input('method','announcement');
                break;
            case 'program':
                set_input('method','program');
                break;
            case 'permission':
                set_input('method','permission');
                break;
            case 'file':
                set_input('method','file');
                break;
            case 'service':
                set_input('method','service');
                break;
            default: set_input('method','list'); break;
        }
        include $CONFIG->pluginspath . 'school/pages/approval.php';
        break;

    case 'teacher':
        switch($page[1]) {
            case 'new': 
                set_input('method','new'); 
                break;
            case 'edit': 
                set_input('method','edit');
                set_input('teacher_guid',$page[2]); 
                break;
            case 'view': 
                set_input('method','view');
                set_input('teacher_guid',$page[2]); 
                break;
            case 'delete': 
                set_input('method','delete');
                set_input('teacher_guid',$page[2]); 
                break;
            default: set_input('method','list'); break;
        }
        include $CONFIG->pluginspath . 'school/pages/teacher.php';
        break;

    case 'register':
        switch($page[1]) {
            case 'new': 
                set_input('method','new'); 
                break;
            case 'edit': 
                set_input('method','edit');
                break;
            case 'student': 
                set_input('method','link_student');
                break;
        }
        include $CONFIG->pluginspath . 'school/pages/register.php';
        break;

    case 'student':
        switch($page[1]) {
            case 'new': 
                set_input('method','new'); 
                break;
            case 'edit': 
                set_input('method','edit');
                set_input('student_guid',$page[2]); 
                break;
            case 'view': 
                set_input('method','view');
                set_input('student_guid',$page[2]); 
                break;
            case 'delete': 
                set_input('method','delete');
                set_input('student_guid',$page[2]); 
                break;
            default: set_input('method','list'); break;
        }
        include $CONFIG->pluginspath . 'school/pages/student.php';
        break;

    case 'class':
        switch($page[1]) {
            case 'new': set_input('method','new'); break;
            case 'edit': 
                set_input('method','edit');
                set_input('class_guid',$page[2]); 
                break;
            case 'view': 
                set_input('method','view');
                set_input('class_guid',$page[2]); 
                break;
            case 'delete': 
                set_input('method','delete');
                set_input('class_guid',$page[2]); 
                break;
            case 'list': set_input('method','list'); break;
        }
        include $CONFIG->pluginspath . 'school/pages/class.php';
        break;

    case 'parent':
        switch($page[1]) {
            case 'edit': 
                set_input('method','edit');
                set_input('parent_guid',$page[2]); 
                break;
            case 'view': 
                set_input('method','view');
                set_input('parent_guid',$page[2]); 
                break;
            case 'delete': 
                set_input('method','delete');
                set_input('parent_guid',$page[2]); 
                break;
            case 'unlink': 
                set_input('method','unlink');
                set_input('parent_guid',$page[2]); 
                set_input('student_guid',$page[3]); 
                break;
            default: set_input('method','list'); break;
        }
        include $CONFIG->pluginspath . 'school/pages/parent.php';
        break;

    case 'program':
        switch($page[1]) {
            case 'new': 
                set_input('method','new'); 
                break;
            case 'edit': 
                set_input('method','edit');
                set_input('program_guid',$page[2]); 
                break;
            case 'view': 
                set_input('method','view');
                set_input('program_guid',$page[2]); 
                break;
            case 'attendance':
                set_input('method','attendance');
                set_input('program_guid',$page[2]);
                break;
            case 'requests':
                set_input('method','requests');
                set_input('program_guid',$page[2]);
                break;
            case 'delete': 
                set_input('method','delete');
                set_input('program_guid',$page[2]); 
                break;
            default: 
                set_input('method','list'); 
                set_input('school_guid',$page[1]);
                break;
        }
        include $CONFIG->pluginspath . 'school/pages/program.php';
        break;

    case 'service':
        switch($page[1]) {
            case 'new': 
                set_input('method','new'); 
                break;
            case 'edit': 
                set_input('method','edit');
                set_input('service_guid',$page[2]); 
                break;
            case 'payment':
                set_input('method','payment');
                set_input('service_guid',$page[2]);
                break;
            case 'view': 
                set_input('method','view');
                set_input('service_guid',$page[2]); 
                break;
            case 'delete': 
                set_input('method','delete');
                set_input('service_guid',$page[2]); 
                break;
            default: 
                set_input('method','list'); 
                set_input('school_guid',$page[1]);
                break;
        }
        include $CONFIG->pluginspath . 'school/pages/service.php';
        break;

    case 'report':
        set_input('type',$page[1]);
        set_input('group',$page[2]);
        include $CONFIG->pluginspath . 'school/pages/report.php';
        break;

    case 'popup_checkout':
        include $CONFIG->pluginspath . 'school/pages/popup_checkout.php';
        break;

    case 'download':
        set_input('file',$page[1]); 
        include $CONFIG->pluginspath . 'school/pages/download.php';
        break;

    case 'manage_members': 
        include $CONFIG->pluginspath . 'school/pages/manage_members.php';
        break;

    case 'sso':
        include $CONFIG->pluginspath . 'school/pages/sso.php';
        break;
}
return true;
}

From the FAQ: "Make sure you include your code in your question". Pastebins, while good if you want to link to the rest of a project, are not allowed here. They make it harder for a reviewer to work because a) it's an extra step and b) the sites are not always accessible (as for me currently). If the code is a reasonable length, please copy it into the post and flag it for moderator attention and I'll be glad to reopen it.

DarkMantis · Answer 1 · 2012-01-26 17:27:17Z

I'm not sure what sort of approach you are taking on this, but I would highly recommend making a couple of classes out of that code.

You've obviously got the idea of functions and methods, so if you can wrap them in an object and use it that way, your code will be much cleaner and easier to read/edit as at the moment your code is very procedural.

Things like these functions:

function student_url($entity) {
        global $CONFIG;
        return $CONFIG->url . 'pg/school/student/view/'.$entity->getGUID();
}

function program_url($entity) {
        global $CONFIG;
        return $CONFIG->url . 'pg/school/program/view/'.$entity->getGUID();
}

function service_url($entity) {
        global $CONFIG;
        return $CONFIG->url . 'pg/school/service/view/'.$entity->getGUID();
}

function class_url($entity) {
        global $CONFIG;
        return $CONFIG->url . 'pg/groups/'.$entity->getGUID();
}

function teacher_url($entity) {
        global $CONFIG;
        return $CONFIG->url . 'pg/profile/'.$entity->username;
}

function parent_url($entity) {
        global $CONFIG;
        return $CONFIG->url . 'pg/school/parent/view/'.$entity->getGUID();
}

Could be made into one dynamic function by specifiying the type of entity that you wish to pass through it to generate the UID or URL.

In my opinion you should definatly look up having a go at creating classes for it as that is 750~ lines long, and you could probably cut it down by about half if you were to write it "properly".

The code itself looks quite good, except the points I have mentioned.

Keep up the good work!

Josay · Answer 2 · 2012-01-27 15:20:19Z

    switch($page[1]) {
        case 'event':
            set_input('method','event');
            break;
        case 'product':
            set_input('method','product');
            break;
        case 'announcement':
            set_input('method','announcement');
            break;
        case 'program':
            set_input('method','program');
            break;
        case 'permission':
            set_input('method','permission');
            break;
        case 'file':
            set_input('method','file');
            break;
        case 'service':
            set_input('method','service');
            break;
        default: set_input('method','list'); break;
    }

Can't you do something like :

page=$page[1];
pages=array('event','product','announcement','program','permission','file','service');
set_input('method', in_array($page,$pages) ? $page : 'list');

The same kind of things applies at different level in your code.

Are saying the way in which it is currently being done is inefficient?
I'm just saying that you can make it easier to read and to maintain by removing all the duplicated logic.Efficiency will stay mostly the same.

asked	1 year ago
viewed	234 times
active	1 year ago

Experienced PHP developer, please REVIEW my code

2 Answers

Your Answer

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

Community Bulletin

Experienced PHP developer, please REVIEW 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 or ask your own question.

Community Bulletin

Related