3
\$\begingroup\$

I have a PHP CodeIgniter website that collects volunteer registrations. One thing I have noticed is that the controllers are getting really big. The controllers have many methods, and each method is fairly long. I believe this could be a code smell, and I would like some help and advice on how to refactor the code.

The website is in production and too big to post all the code. I will pick a sample page to post here so you can get an idea. I'm looking for broad feedback on how to improve the organization of the code. For example:

  • Idea 1 - Should I move some of this code to a new layer between the controller and the model? I've seen this recommended in some places. I believe it's called a domain layer.
  • Idea 2 - Should I break this entire method (and other big methods) off into their own classes? Since I'm using CodeIgniter, I guess these would be library classes?
  • Idea 3 - Should I make more use of private methods within the giant controller class? Won't shrink the size of the controller, but might make some of the code more readable. Or maybe not, since I'd be scrolling all over the page to find code, instead of just reading it linearly.
  • Idea 4 - Should I try to move more code into the models? My models are a similar size to the controllers.
  • Idea 5 - The code works and I (the only developer) can read it just fine. Should I just leave it alone? YAGNI? Keep complexity down?
  • I have a lot of controller methods that start out with form validation code. What is the best way to handle that?
  • Bonus question: What are good ways to test a website? I currently use iMacros browser plugin and run scripts that simulate typing, mouse clicks, submitting forms, clicking links, etc. I have heard of unit testing but am not sure where to start with that. Is it worth it to write a bunch of unit tests?
  • Other ideas/feedback?

Screenshot

screenshot of website

Code

public function edit_group($group_id_and_text = NULL)
{
    $this->data = set_page_title('Edit Group', $this->data);

    $this->data = $this->_get_group_data_or_throw_error_page($group_id_and_text, $this->data, '/managers/edit_group/');

    $this->data['list_of_shifts'] = $this->shift_model->get_race_shifts_with_enrolled_plus_groups_fields($this->data['race']['race_id']);

    $this->data['list_of_group_shifts'] = $this->group_shift_model->get_group_shifts_for_group($this->data['group']['group_id']);

    // output format is $shift_id => $number_promised
    $this->data['list_of_group_shifts_as_list'] = $this->group_shift_model->get_group_shifts_as_list($this->data['group']['group_id'], $this->data['race']['race_id']);

    $this->data['list_of_shifts_to_highlight'] = highlight_shifts_that_need_more_groups($this->data['list_of_shifts']);

    $this->data['list_of_volunteers_with_shifts'] = $this->volunteer_shift_model->get_volunteers_by_race_and_group($this->data['race']['race_id'], $this->data['group']['group_id']);

    $this->data['list_of_volunteers_without_shifts'] = $this->volunteer_shift_model->get_volunteers_with_no_shifts_filter_by_group($this->data['race']['race_id'], $this->data['group']['group_id']);

    $this->data['list_of_volunteers'] = array_merge($this->data['list_of_volunteers_with_shifts'], $this->data['list_of_volunteers_without_shifts']);

    $this->form_validation->set_rules('group_name', 'Group Name', 'trim|required|max_length[200]');

    $this->form_validation->set_rules('shift_id[]', 'Volunteer Shift', 'trim|valid_volunteer_group_shift_for_admin[' . $this->data['race']['race_id'] . ']');

    $this->form_validation->set_rules('group_notes', 'Group Notes', 'trim|max_length[1000]');
    $this->form_validation->set_rules('group_send_reminder_emails', 'Reminder Emails', 'trim|required|in_list[0,1]');
    $this->form_validation->set_rules('fix_shifts', 'Fix Volunteer Shifts', 'trim|in_list[1]');
    $this->form_validation->set_rules('email_group_leader', 'Email Group Leader', 'trim|in_list[1]');
    $this->form_validation->set_rules('delete_group', 'Delete Group', 'trim|in_list[1]');
    $this->form_validation->set_rules('delete_group_members', 'Delete Group Members', 'trim|in_list[1]');
    $this->form_validation->set_rules('group_firm', 'Mark As Firm', 'trim|in_list[1]');

    // Note: SQL WHERE is case insensitive, which is good in this case
    $this->data['group_for_duplicate_check'] = $this->group_model->get_group_by_race_and_group_name(
        $this->data['race']['race_id'],
        $this->input->post('group_name')
    );

    if ($this->form_validation->run() === FALSE)
    {
        load_page_with_event_nav('managers/edit_group', $this->data);
    }
    elseif ( $this->data['group_for_duplicate_check'] && $this->data['group_for_duplicate_check']['group_id'] != $this->data['group']['group_id'] )
    {
        add_message('error', 'A group with this name already exists. <a href="/managers/edit_group/' . $this->data['group_for_duplicate_check']['group_uri'] . '">Click here</a> to view and edit the existing group.');

        load_page_with_event_nav('managers/edit_group', $this->data);
    }
    else
    {
        $group_leader_volunteer_id = $this->data['group']['group_leader_volunteer_id'];

        // Putting delete_group_members before delete_group so that the soft deleted volunteers keep their group_id. Will be helpful if I have to undo the soft delete.
        if ( $this->input->post('delete_group_members') )
        {
            $volunteer_ids = $this->data['list_of_volunteers'];

            if ( $volunteer_ids ) {         
                $volunteer_ids = sql_make_list_from_sql_result_array($volunteer_ids, 'volunteer_id');

                $volunteer_ids = mv_eliminate_duplicates($volunteer_ids);

                $this->volunteer_model->soft_delete_volunteer($volunteer_ids);

                add_message('success', '"' . html_escape($this->data['group']['group_name']) . '"\'s group members were successfully deleted from the group AND the volunteer database.');
            }
        }

        if ( $this->input->post('delete_group') )
        {
            $this->group_model->soft_delete_groups($this->data['group']['group_id']);

            // If group_leader_volunteer_id got deleted because the volunteer got soft deleted, AND the group is getting deleted, restore the group_leader_volunteer_id so that email_list can display deleted group leaders.
            $this->group_model->set_group_leader($this->data['group']['group_id'], $group_leader_volunteer_id);

            add_message('success', '"' . html_escape($this->data['group']['group_name']) . '" was successfully deleted.');
        }
        else
        {
            $this->group_model->edit_group($this->data);

            // refresh some variables needed down here
            $this->data['group'] = $this->group_model->get_group_by_id($this->data['group']['group_id']);

            $this->data['volunteer'] = $this->volunteer_model->get_volunteer_by_id($this->data['group']['group_leader_volunteer_id']);

            $this->data['list_of_group_shifts'] = $this->group_shift_model->get_group_shifts_for_group($this->data['group']['group_id']);

            $shifts_to_compare = $this->data['list_of_group_shifts_as_list'];

            /*
                PHP's array compare is extremely loose.
                1) It compares array contents, not references.
                2) It compares across types. For example, 0 and '0' are seen as the same thing.
                3) The order of the array keys doesn't matter, so we don't need to sort them.
            */
            if ( $this->input->post('shift_id') != $shifts_to_compare )
            {
                $this->group_shift_model->hard_delete_groups_shifts($this->data['group']['group_id']);

                foreach ( $_POST['shift_id'] as $shift_id => $value )
                {
                    if ( $value != 0 )
                    {
                        $this->group_shift_model->add_shift(
                            $this->data['group']['group_id'],
                            $shift_id,
                            $value,
                            $this->data
                        );
                    }
                }
            }

            // refresh again
            $this->data['list_of_group_shifts'] = $this->group_shift_model->get_group_shifts_for_group($this->data['group']['group_id']);

            $list_of_group_shifts = sql_make_list_from_sql_result_array($this->data['list_of_group_shifts'], 'shift_id');

            // Make sure the group leader is enrolled in all the group's shifts. This is important so that the group leader receives the volunteer instructions for each of this group's shifts.
            if ( $this->data['volunteer'] )
            {
                foreach ( $list_of_group_shifts as $key => $shift_id )
                {
                    $shift = $this->volunteer_shift_model->get_shift_by_volunteer_id_and_shift_id($this->data['volunteer']['volunteer_id'], $shift_id);

                    if ( ! $shift )
                    {
                        $this->volunteer_shift_model->add_shift(
                            $this->data['volunteer']['volunteer_id'],
                            $shift_id,
                            $this->data['auth']['manager']['manager_id']
                        );
                    }
                }
            }

            add_message('success', '"<a href="/managers/edit_group/' . $this->data['group']['group_uri'] . '">' . html_escape($this->data['group']['group_name']) . '</a>" was successfully edited.');

            if ( $this->input->post('fix_shifts') == 1 )
            {
                // ****** FIX_SHIFTS_REMOVE ******
                foreach ( $this->data['list_of_volunteers_with_shifts'] as $key => $volunteer_shift )
                {
                    if ( !in_array($volunteer_shift['shift_id'], $list_of_group_shifts) )
                    {
                        $this->volunteer_shift_model->hard_delete_one_volunteer_one_shift($volunteer_shift['volunteer_id'], $volunteer_shift['shift_id']);
                    }
                }

                // ****** FIX_SHIFTS_ADD ******
                $this->data['list_of_volunteers_not_joined_with_volunteer_shifts'] = $this->volunteer_model->get_volunteers_by_race_and_group($this->data['race']['race_id'], $this->data['group']['group_id']);

                $this->data['list_of_volunteer_shifts_for_this_race'] = $this->volunteer_shift_model->get_volunteers_by_race_order_by_name($this->data['race']['race_id']);

                foreach ( $this->data['list_of_volunteers_not_joined_with_volunteer_shifts'] as $key => $volunteer )
                {
                    foreach ( $list_of_group_shifts as $key => $shift_id )
                    {
                        $shift_already_exists = sql_search_result_array_contains_key1_value1_key2_value2(
                            $this->data['list_of_volunteer_shifts_for_this_race'],
                            'volunteer_id',
                            $volunteer['volunteer_id'],
                            'shift_id',
                            $shift_id
                        );

                        if ( ! $shift_already_exists )
                        {
                            $this->volunteer_shift_model->add_shift(
                                $volunteer['volunteer_id'],
                                $shift_id,
                                $this->data['auth']['manager']['manager_id']
                            );
                        }
                    }
                }

                $this->group_shift_model->fix_more_enrolled_vols_than_estimated_vols($this->data['group']['group_id']);
            }

            if ( $this->input->post('email_group_leader') == 1 )
            {
                // return true for non-NULL, non-zero
                if ( $this->data['group']['group_leader_volunteer_id'] )
                {
                    $this->data['list_of_this_groups_shifts'] = $this->shift_model->get_group_shifts($this->data['group']['group_id']);

                    send_group_confirmation_email($this->data, $this->data['volunteer']['volunteer_email']);

                    add_message('success', 'Also, we e-mailed the group leader a group confirmation e-mail.');
                }
                else
                {
                    add_message('error', 'You requested that we send a confirmation e-mail, but we were unable to because a volunteer group leader was not provided.');
                }
            }
        }

        $this->shift_model->recalculate_shift_stats($this->data['race']['race_id']);

        redirect_and_die('/managers/group_report/' . $this->data['race']['race_uri']);
    }
}
\$\endgroup\$
  • 1
    \$\begingroup\$ in idea 3, you are making wrong assumption that you will have to scroll a lot. No, you will actually read it much easier. Put validation into own method, then you read the code And see: here Is validation taking place. Ok now you either think you are not interested in validation piece of code so you continue reading the very next line. Or you Are interested in validation, So you jump to that method and all you see now is validation which is all you care of. \$\endgroup\$ – slepic Apr 2 at 20:06
  • \$\begingroup\$ Ideas 1 and 2 are basically the same. \$\endgroup\$ – slepic Apr 2 at 20:10
  • \$\begingroup\$ @slepic All these years I didn't know about "Go To Definition" command in IDE. F12 in VS Code. That will make jumping around big classes much easier. Thank you for pointing it out. \$\endgroup\$ – RedDragonWebDesign Apr 3 at 2:18
2
+50
\$\begingroup\$

There are a lot of possibilities laid out here, but I think I would lean towards:

Idea 3 - Should I make more use of private methods within the giant controller class? Won't shrink the size of the controller, but might make some of the code more readable. Or maybe not, since I'd be scrolling all over the page to find code, instead of just reading it linearly.

I would recommend taking this approach. Looking at the sample controller method I would separate chunks into separate methods, or consider making separate controllers that would have unique methods (part of Idea 2 I guess). Ask yourself what the chunks are doing - e.g.

  • setting data
  • setting form validation rules
  • loading page with event nav and/or adding error/success message

Having smaller, more atomic methods should allow for better unit testing. This is congruent with the Single Responsibility Principle. Like I mentioned in my review of your chess code it is wise to limit methods to 15-20 lines. In this presentation about cleaning up code Rafael Dohms talks about limiting the indentation level to one per method and avoiding the else keyword. (see the slides here).

I'm not very familiar with CodeIgniter but see the documentation for CI4 contains a section HTTP Feature Testing. I'd recommend reading through that page - hopefully something there will be useful.

| improve this answer | |
\$\endgroup\$
  • 1
    \$\begingroup\$ Thank you very much for your comment Sam. I actually remember this video from your PHP chess code review, and I used it and your other tips to refactor a lot of my PHP chess program. I gave the video another watch start to finish today and I refreshed myself on many of the concepts. That converting "else" into "returns" idea is excellent, as is keeping indentation down. \$\endgroup\$ – RedDragonWebDesign Apr 3 at 2:25
  • 1
    \$\begingroup\$ I am still on CI3. Exciting to see CI4 (released in Feb 2020) has some testing features built in. Of course, the core problem with testing a website is how inter-connected everything on a website is. Most PHP controller methods on my website rely heavily on reading and writing the SQL database. And also the HTML needs testing, too. Unit testing is a no brainer with something like a ChessBoard class that doesn't touch a database, but is much harder for a website that is constantly touching the SQL database. \$\endgroup\$ – RedDragonWebDesign Apr 3 at 2:27
0
\$\begingroup\$

Somebody on another website posted a good idea and I want to share it. They suggested that I make each webpage its own class. Then any private methods that support that webpage are grouped together in the same file, and other webpages and private methods get their own files and aren't cluttering things up.

I could use folders to keep similar pages grouped together.

| improve this answer | |
\$\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.