2

As an semi-experienced procedural PHP developer, my OOP still needs a lot of work. I am still working through SOLID principles and other theories and guidelines of OOP, and am stuck with something that just looks UGLY to say the least, and definitely does not confirm to SOLID.

I am busy redoing my in-house CMS, using CodeIgniter 3.0.x and MaterializeCSS for the front-end.

Here is the bit of code in question that simply does not work for me:

public function add($uid)
{
    $data['userdata'] = $this->usercontact_model->get_user($uid);
    if (empty($data['userdata']))
    {
        return false;
    }
    else
    {
        $temp = $this->usercontact_model->get_contact_types();
        $temp_options = array();
        if (!empty($temp))
        {
            foreach ($temp as $tk => $tv)
            {
                $temp_options[$tk]['value'] = $tv['id'];
                $temp_options[$tk]['display'] = $tv['name'];
            }
        }            
        $data['sidebar'] = array(
            0 => array(
                'type' => 'accordion',
                'content' => array(
                    0 => array(
                        'icon' => 'settings',
                        'active' => 'active',
                        'heading' => 'Page actions',
                        'prompt' => lang('user_contacts_page_actions_head') . '<br />&nbsp;<br />',
                        'body' => '
                            &raquo;&nbsp;<a href="' . base_url('user/usercontact/list_contacts') . '/' . $this->session->userdata('id') . '">' . lang('user_contacts_return_contacts') . '</a><br />
                            &raquo;&nbsp;<a href="' . base_url('user/login/control_panel') . '">' . lang('user_contacts_return_cpanel') . '</a><br />
                        '
                    )
                )
            )               
        ); 
        $data['open_data'] = array(
            'action' => '',
            'method' => 'post',
            'hidden' => array(
                'user_id' => $this->session->userdata('id')
            )
        );   
        $data['close_data'] = array(
            'datepicker_init' => 'true',
            'select_months' => 'true',
            'select_years' => 15,
            'format' => 'yyyy-mm-dd',
            'clockpicker_init' => 'true'
        );        
        $data['contact_type_data'] = array(
            'id' => 'contact_type_select',
            'heading' => 'user_contact_type',
            'options' => $temp_options
        );
        $data['active_data'] = array(
            'id' => 'active_select',
            'heading' => 'user_contact_active',
            'options' => array(
                0 => array(
                    'value' => 'Yes',
                    'display' => lang('user_contact_yes')
                ),
                1 => array(
                    'value' => 'No',
                    'display' => lang('user_contact_no')
                )
            )
        );            
        $data['value_data'] = array(
            'id' => 'value',
            'validate' => 'validate',
            'placeholder' => 'Content',
            'label' => 'user_contact_label_value'
        );
        $data['vacation_from_data'] = array(
            'id' => 'vacation_from',
            'placeholder' => 'Select vacation start date',
            'label' => 'user_contact_vacation_from'
        );
        $data['vacation_to_data'] = array(
            'id' => 'vacation_to',
            'placeholder' => 'Select vacation end date',                
            'label' => 'user_contact_vacation_to'
        );            
        $data['time_from_data'] = array(
            'id' => 'time_from',
            'placeholder' => 'Select available start time',                
            'label' => 'user_contact_time_from'
        );
        $data['time_to_data'] = array(
            'id' => 'time_to',
            'placeholder' => 'Select available end time',                
            'label' => 'user_contact_time_to'
        );  
        $data['submit_data'] = array(
            'id' => 'submit_add',
            'label' => lang('user_contact_save'),
        );                        
        $posts = $this->input->post();
        if (empty($posts))
        {
            $this->core->render_view('template_admin', 'usercontact/add', $data);
        }
        else
        {
            $this->core->render_view('template_admin', 'usercontact/add', $data);
            preint($posts);                
        }
    }
}

Now - I have not built in validation to this yet, but that will make this undoubtedly more ugly using my current single function approach.

preint() is just a function I created that does a print_r wrapped in <pre></pre>

Specific questions:

  1. How, using CI 3.0.x, can I make this more SOLID compliant?
  2. How can I make this more readable?
  3. How can I shorten this? Move things such as validation and creating the from fields and sidebar arrays to separate functions/classes?

Answers or suggestions to any or all of the above would be greatly appreciated.

3
  • All those arrays that set the form fields and form meta data have now been moved to separate private functions within the same class. Not sure if this is the right way to go, but it does look a lot better already... Commented Apr 12, 2016 at 10:27
  • 3
    What are the differences between Stack Overflow, Programmers, and Code Review? Commented Apr 28, 2016 at 2:17
  • 3
    just for starters, you could get rid of the first }else{, cause the if body already returns a false so the method will not be executed any further if the condition is true. This makes the else body a bit unnecessary. To my opinion it's a bit more readible and less error-prone without the else block. And maybe, if you use the same hardcoded array more than once, you could put that one in a separate method or file. In that case you could put that array in a variable whenever you need it and just modify the keys and values you need to change. Commented May 2, 2016 at 14:22

1 Answer 1

1

Each of those $data sub-structures can probably be abstracted into their own classes. Find the similarities between them and make a family tree of classes. You might even be able to reuse a single general purpose class for them.

You should pass in the user data to the method, not look it up inside. Every method should be single-purpose and have it's dependencies given to it. This is how you build layers and separate concerns.

$temp_options is only used once, so move it closer to where it's used. Or since you should make the thing using it into its own class anyway, do the re-keying in that object.

Don't hard-code HTML. That's what the view is for. Make the view do the work of constructing links and formatting, etc.

Make the user object (if you even have a user object) return the appropriate URLs. That way the view can just call $user->getControlPanelURL(), etc.

You could even represent URLs as objects, and give them __toString() methods so you can echo them directly from the view.

It can be difficult getting used to OOP when you come from a procedural background. Just think of LEGOs.

Also this book is required reading for any OOP developer. https://en.wikipedia.org/wiki/Design_Patterns

1
  • 1
    Thank you for a very full-featured answer! I have received a lot of suggestions in comments, but none so complete as this. Thank you! Commented Aug 15, 2016 at 8:38

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service and acknowledge you have read our privacy policy.

Start asking to get answers

Find the answer to your question by asking.

Ask question

Explore related questions

See similar questions with these tags.