#1
  1. A Change of Season
    Devshed Frequenter (2500 - 2999 posts)

    Join Date
    Mar 2004
    Location
    Next Door
    Posts
    2,675
    Rep Power
    171

    Tips required to organize class...


    Hello;

    This is getting too messy. I appreciate if you give me some tips to organize my code better. Maybe this is how its supposed to be like! Who knows? Probably eoreo.
    Thank you.
    PHP Code:
    class List_your_place extends CI_Controller {

        protected 
    $invalid_year false;
        protected 
    $invalid_month false;
        protected 
    $invalid_day false;
        protected 
    $invalid_date false;
        
        
        public function 
    __construct()
            {
                
    parent::__construct();
                
    define('EIGHTEEN_YEARS_AGO'date('Y'strtotime('-17 years'strtotime(date('Y')))));
            }
            
        public function 
    index()
            {
                if(
    $this->input->post('submit'))
                    {
                        
    $this->validate();
                    }
                
    $this->cities_dropdown();
                
    $this->assign_values();
            }
        protected function 
    validate()
            {
                
    $this->form_validation->set_rules('ad_title''Title''required|min_length[5]|max_length[30]');
                
    $this->form_validation->set_rules('search''City''required');
                
    $this->form_validation->set_rules('ad_suburb''Suburb''required|max_length[30]');
                
    $this->form_validation->set_rules('ad_address''Address''required|min_length[5]|max_length[30]');
                
    $this->form_validation->set_rules('altFormat''Date''required|callback_valid_date');
                
    $this->form_validation->set_rules('ad_rent''Weekly Rent''required|callback_valid_rent');
                
    $this->form_validation->set_rules('ad_bond''Security Bond''required|callback_valid_bond');
                
    $this->form_validation->set_rules('ad_phone''Phone''required|min_length[5]|max_length[30]|alpha_numeric');
                
    $this->form_validation->set_rules('ad_email''Email''required|valid_email|callback_already_member');
                
    $this->form_validation->set_rules('ad_password''Password''required|min_length[5]|max_length[30]');
                
    $this->form_validation->set_rules('ad_yob''Year of Birth''numeric|required|less_than['.EIGHTEEN_YEARS_AGO.']');
                
    $this->form_validation->set_rules('ad_sex''Gender''required|exact_length[1]');
                
    $this->form_validation->set_rules('ad_bt''Building Type''required');
                
    $this->form_validation->set_rules('ad_smoking_habbit''Smoking habbit''required|exact_length[1]');
                
    $this->form_validation->set_rules('ad_description''Ad description''required|min_length[20]|max_length[1000]');
                if(
    $this->form_validation->run() == FALSE)
                    {
                        
    $this->form_validation->set_error_delimiters('<span class="validation_error">''</span>');
                        
    $errors[]=validation_errors();
                        
    $this->smarty->assign('errors'$errors);
                    }
                else    
                    {
                        
    $this->submit_ad();
                    }
            }
        public function 
    already_member($email)
            {
                
    $this->load->model('check_member_model');
                if(
    $this->check_member_model->check($email))
                    {
                        return 
    true;
                    }
                else
                    {
                        
    $this->form_validation->set_message('already_member''This email address is already in the system! If it belongs to you please click <a href=\''.base_url().'log\'>here</a> to log in.');
                        return 
    false;
                    }
            }
        protected function 
    submit_ad()
            {
                
    $this->load->model('members_model');//Insert this member
                
    $new_member_id $this->members_model->add_member();
                
                
    $this->load->model('ad_model');//Insert ad 
                
    $sucees_id $this->ad_model->insert_ad($new_member_id);
                
                
    $config['mailtype'] = 'html';
                
    $this->email->initialize($config);
                
                
    $this->email->from('support@flatmatescenter.com''FlatmatesCenter.com');
                
    $this->email->to($this->input->post('ad_email'));
                
    $this->email->subject('Welcome to Flatmatescenter');
                
                
    $output $this->smarty->fetch('email_new_ad_view.tpl');
                
    $output str_replace("#title#",$this->input->post('ad_title'), $output);
                
    $output str_replace("#address#"$this->input->post('ad_address'), $output);
                
    $output str_replace("#suburb#"$this->input->post('ad_suburb'), $output);
                
    $output str_replace("#rent#""$".$this->rent$output);
                
    $output str_replace("#bond#""$".$this->bond$output);
                
    $output str_replace("#A_ID#"$sucees_id$output);
                
    $this->email->message($output);   
                
    $this->email->send();
                
    redirect(base_url().'success''location'301); 
            }
        public function 
    valid_rent($rent)
            {
                
    $rent preg_replace('#\W#'''$rent);
                if(
    $rent<|| $rent>3000 || !is_numeric($rent))
                    {
                        
    $this->form_validation->set_message('valid_rent''Rent must be between $0 - $3000');
                        return 
    false;
                    }
                else    
                    {
                        
    $this->rent=$rent;
                        return 
    true;
                    }
            }
        public function 
    valid_bond($bond)
            {
                
    $bond preg_replace('#\W#'''$bond);
                if(
    $bond<|| $bond>5000)
                    {
                        
    $this->form_validation->set_message('valid_bond''Bond must be between $0 - $5000');
                        return 
    false;
                    }
                else    
                    {
                        
    $this->bond=$bond;
                        return 
    true;
                    }
            }
        public function 
    valid_date($date)
            {
                
    $date_format explode('-',$date);
                if(!
    is_numeric($date_format['0']) || $date_format['0']<date('Y') || $date_format['0']>strtotime("+2 years"))
                    {
                        
    $this->invalid_year=true;
                    }
                if(!
    is_numeric($date_format['1']) || $date_format['1']<|| $date_format['1']>12)
                    {
                        
    $this->invalid_month=true;
                    }
                if(!
    is_numeric($date_format['2']) || $date_format['2']<|| $date_format['2']>31)
                    {
                        
    $this->invalid_day=true;
                    }
                if(!
    checkdate($date_format['1'],$date_format['2'],$date_format['0']))
                    {
                        
    $this->invalid_date=true;
                    }
                 if(
    $this->invalid_year==true || $this->invalid_month==true || $this->invalid_day==true || $this->invalid_date==true)
                    {
                        
    $this->form_validation->set_message('valid_date''Invalid date available');
                        return 
    false;
                    } 
            }
        protected function 
    cities_dropdown()
            {
                    
    $this->load->model('cities_model');
                    
    $cities $this->cities_model->load_cities();
                    foreach(
    $cities as $val)
                        {
                            
    $australia[] = array('name'=>$val['name'], 'id'=>$val['id'], 'number_of_ads'=>$val['number_of_ads']);
                        }
                    
    $this->smarty->assign("australia"$australia);
                    
    $form_attributes = array('class' => 'form_left');
                    
    $form_open form_open('list_your_place'$form_attributes);
                    
    $form = array('open'=>$form_open'close'=>form_close());
                    
    $this->smarty->assign('form'$form);
                    
    $this->smarty->assign('searched'$this->input->post('search'));
            }
        protected function 
    assign_values()
            {
                
    $this->smarty->assign("title"'List your place or your extra room for rent or share');
                
    $eigtheen_years_ago date('Y'strtotime('-18 years'strtotime(date('Y'))));
                
    $this->smarty->assign("eigtheen_years_ago"$eigtheen_years_ago);
                
    $this->smarty->view('header');
                
    $this->smarty->view('list_your_place');
            }

  2. #2
  3. Mad Scientist
    Devshed Expert (3500 - 3999 posts)

    Join Date
    Oct 2007
    Location
    North Yorkshire, UK
    Posts
    3,661
    Rep Power
    4124
    Every time you see lots of calls that look the same then stick them in a loop. The values would then come from a data store of some kind (db, config file, XML, etc) - ie abstract it.

    Similarly with all the replacements. I'm sure that's not the only code you have that does something similar do build a generic function/class and put that in your code library

    Think about responsibilities. This class should only be responsive for the management of the high level stuff. Put the lower level stuff (inc values) in lower down classes that are called by the controller

    In general, if a class has more than 50 lines of code then consider splitting it - what could be done elsewhere? What could be generalised for reuse?
    I said I didn't like ORM!!! <?php $this->model->update($this->request->resources[0])->set($this->request->getData())->getData('count'); ?>

    PDO vs mysql_* functions: Find a Migration Guide Here

    [ Xeneco - T'interweb Development ] - [ Are you a Help Vampire? ] - [ Read The manual! ] - [ W3 methods - GET, POST, etc ] - [ Web Design Hell ]
  4. #3
  5. A Change of Season
    Devshed Frequenter (2500 - 2999 posts)

    Join Date
    Mar 2004
    Location
    Next Door
    Posts
    2,675
    Rep Power
    171
    Originally Posted by Northie
    Every time you see lots of calls that look the same then stick them in a loop. The values would then come from a data store of some kind (db, config file, XML, etc) - ie abstract it.

    Similarly with all the replacements. I'm sure that's not the only code you have that does something similar do build a generic function/class and put that in your code library

    Think about responsibilities. This class should only be responsive for the management of the high level stuff. Put the lower level stuff (inc values) in lower down classes that are called by the controller

    In general, if a class has more than 50 lines of code then consider splitting it - what could be done elsewhere? What could be generalized for reuse?
    Would you please show me an example using my code above? Perhaps splitting it and taking one of the methods out as another class.
    Thank you.
  6. #4
  7. No Profile Picture
    Contributing User
    Devshed Newbie (0 - 499 posts)

    Join Date
    Dec 2012
    Location
    Ithaca
    Posts
    68
    Rep Power
    2
    One thing I can think of is to remove the responsibility for your class to handle validation. Instead, create a class called Validator with methods such as validateRent($rent), validateBond($bond) and validateDate($date) to handle this specialized task.

    This way your controller does not look like a God object that is trying to do way too many things. On the other hand, your validator class may be reusable in other controller classes. You may want to store a validator object as a property in your controller class.
  8. #5
  9. Mad Scientist
    Devshed Expert (3500 - 3999 posts)

    Join Date
    Oct 2007
    Location
    North Yorkshire, UK
    Posts
    3,661
    Rep Power
    4124
    I aint going to write your code for you, but you're going to struggle if you can't makle loops out of things that are similar, for example:

    PHP Code:
    //
    /*
        $rules = array(
            array('ad_title', 'Title', 'required|min_length[5]|max_length[30]'), 
            array('search', 'City', 'required'), 
            array('ad_suburb', 'Suburb', 'required|max_length[30]'), 
            array('ad_address', 'Address', 'required|min_length[5]|max_length[30]'), 
            array('altFormat', 'Date', 'required|callback_valid_date'), 
            array('ad_rent', 'Weekly Rent', 'required|callback_valid_rent'), 
            array('ad_bond', 'Security Bond', 'required|callback_valid_bond'), 
            array('ad_phone', 'Phone', 'required|min_length[5]|max_length[30]|alpha_numeric'), 
            array('ad_email', 'Email', 'required|valid_email|callback_already_member'), 
            array('ad_password', 'Password', 'required|min_length[5]|max_length[30]'), 
            array('ad_yob', 'Year of Birth', 'numeric|required|less_than['.EIGHTEEN_YEARS_AGO.']'), 
            array('ad_sex', 'Gender', 'required|exact_length[1]'), 
            array('ad_bt', 'Building Type', 'required'), 
            array('ad_smoking_habbit', 'Smoking habbit', 'required|exact_length[1]'), 
            array('ad_description', 'Ad description', 'required|min_length[20]|max_length[1000]')
        );
    //*/

    $rules $form->validator->getRules(); //returns something like the above structure

    foreach($rules as $rule) {
        
    $this->form_validation->set_rules($rule[0],$rule[1],$rule[2],); 

    and please please please do not ask me what is in the form, validator or getRules class/functions i have put in as an example - they are there as an example only - you should be the one who decide what code too put in them if you want.

    Also, Hall of Famer's point of having a validator class is also valid. I would suggest having a set of library classes that manage your forms (rendering and validating). This would be called upon by your controller which would define (directly or by way of a model) which fields to use in the form and then when valid data is returned the controller would then instruct a model to handle (ie insert/update/calculate) the data
    Last edited by Northie; December 3rd, 2012 at 03:20 AM.
    I said I didn't like ORM!!! <?php $this->model->update($this->request->resources[0])->set($this->request->getData())->getData('count'); ?>

    PDO vs mysql_* functions: Find a Migration Guide Here

    [ Xeneco - T'interweb Development ] - [ Are you a Help Vampire? ] - [ Read The manual! ] - [ W3 methods - GET, POST, etc ] - [ Web Design Hell ]
  10. #6
  11. No Profile Picture
    Contributing User
    Devshed Newbie (0 - 499 posts)

    Join Date
    Dec 2012
    Location
    Ithaca
    Posts
    68
    Rep Power
    2
    Yeah you are absolutely right, looks like the OP's code fits in the category of God Object Anti-Pattern. I had that problem too when I first learned OOP, it was actually much worse than OP's controller lol.
  12. #7
  13. A Change of Season
    Devshed Frequenter (2500 - 2999 posts)

    Join Date
    Mar 2004
    Location
    Next Door
    Posts
    2,675
    Rep Power
    171
    Originally Posted by Hall of Famer
    Yeah you are absolutely right, looks like the OP's code fits in the category of God Object Anti-Pattern. I had that problem too when I first learned OOP, it was actually much worse than OP's controller lol.
    Hello, thank you. The controller is now down to 60 lines. What seems to be a solution (as mentioned above) is to load parts of the code as my own library. It works but I faced a small problem here.

    I am trying to load multiple libraries in to my class but I get error!
    No matter which one is first, the first one loads fine and the second on doesn’t!
    what am I doing wrong here? this is my code and below is the error that I get.
    PHP Code:
     //Load First Library (which loads fine)
    $this->load->library('cities');
    $australia $this->cities->cities_dropdown(); 
    //Load Second Library (which which does not load!)
    $this->load->library('test');
    $australia $this->test->tester(); 
    The error:

    A PHP Error was encountered

    Severity: Notice

    Message: Undefined property: List_your_place::$test

    Filename: controllers/list_your_place.php

    Line Number: 21

    ( ! ) Fatal error: Call to a member function tester() on a non-object
  14. #8
  15. Mad Scientist
    Devshed Expert (3500 - 3999 posts)

    Join Date
    Oct 2007
    Location
    North Yorkshire, UK
    Posts
    3,661
    Rep Power
    4124
    Ah, a factory pattern where one is not needed

    I'm not a fan of that use of things

    Me, I would just write

    $cities = new \libs\Cities($name_of_form);
    $foo = $cities->cities_dropdown();

    and then

    $test = new \libs\test;
    $bar = $test->tester();

    my autoload function(s) would auto include it and it would be ready for use

    To me, this looks like this a PHP4 backwards compatability hangover
    I said I didn't like ORM!!! <?php $this->model->update($this->request->resources[0])->set($this->request->getData())->getData('count'); ?>

    PDO vs mysql_* functions: Find a Migration Guide Here

    [ Xeneco - T'interweb Development ] - [ Are you a Help Vampire? ] - [ Read The manual! ] - [ W3 methods - GET, POST, etc ] - [ Web Design Hell ]
  16. #9
  17. A Change of Season
    Devshed Frequenter (2500 - 2999 posts)

    Join Date
    Mar 2004
    Location
    Next Door
    Posts
    2,675
    Rep Power
    171
    Originally Posted by Northie
    Ah, a factory pattern where one is not needed

    I'm not a fan of that use of things

    Me, I would just write

    $cities = new \libs\Cities($name_of_form);
    $foo = $cities->cities_dropdown();

    and then

    $test = new \libs\test;
    $bar = $test->tester();

    my autoload function(s) would auto include it and it would be ready for use

    To me, this looks like this a PHP4 backwards compatability hangover
    What about Using Your Class!!!
  18. #10
  19. Mad Scientist
    Devshed Expert (3500 - 3999 posts)

    Join Date
    Oct 2007
    Location
    North Yorkshire, UK
    Posts
    3,661
    Rep Power
    4124
    That's one of the reasons why I came to the conclusion that I could probably write a framework better suited to my needs. So I did.

    I can't help you debug your code, but the cause is that the object you are attempting to create is either not being created or you are creating something else,

    "Call to a member function tester() on a non-object"

    PHP Code:
    $this->load->library('test'); 
    $australia $this->test->tester(); 
    I guess the first line tries to load the test class and makes an instance of it available in $this->test (test being the name of the class).

    However, $this->test is not an object, (according to the error, or at the very least does not have the method tester()) so do a var dump on it,

    PHP Code:
    var_dump($this->test); 
    I expect you'll get either null or boolean false.

    If so, consult the CI docs and/or google for why loading your own libraries gives the result you get
    I said I didn't like ORM!!! <?php $this->model->update($this->request->resources[0])->set($this->request->getData())->getData('count'); ?>

    PDO vs mysql_* functions: Find a Migration Guide Here

    [ Xeneco - T'interweb Development ] - [ Are you a Help Vampire? ] - [ Read The manual! ] - [ W3 methods - GET, POST, etc ] - [ Web Design Hell ]
  20. #11
  21. A Change of Season
    Devshed Frequenter (2500 - 2999 posts)

    Join Date
    Mar 2004
    Location
    Next Door
    Posts
    2,675
    Rep Power
    171
    Originally Posted by Northie
    If so, consult the CI docs and/or google for why loading your own libraries gives the result you get
    Thanks for the tips. It's getting better. I feel I am on the right track. Saying that I know there is still a lot I can improve. Dont hesitate to mention.
    PHP Code:
    class List_your_place extends CI_Controller {
        
        public function 
    __construct()
            {
                
    parent::__construct();
                
                
    $this->load->library('cities');//Build cities drop down
                
    $australia $this->cities->cities_dropdown();
                
    $this->smarty->assign("australia"$australia);
                
    $this->load->library('new_share/build_form','','build_form');//Build register form
                
    $this->build_form->index();
            }
        public function 
    index()
            {
                if(
    $this->input->post('submit'))
                    {
                        
    $this->validate();//Validate if form submit
                    
    }
                
    $this->smarty->assign("title"'List your place or your extra room for rent or share');
                
    $eigtheen_years_ago date('Y'strtotime('-18 years'strtotime(date('Y'))));
                
    $this->smarty->assign("eigtheen_years_ago"$eigtheen_years_ago);
                
    $this->smarty->view('header');
                
    $this->smarty->view('list_your_place');
            }
        protected function 
    validate()
            {
                
    $this->load->library('new_share/validations','','share_validation');
                
    $errors $this->share_validation->validate_ad_have();
                 if(
    $errors)
                    {
                        
    $this->smarty->assign('errors'$errors);
                    }
                else
                    {
                        
    $this->submit_ad();
                        
    redirect(base_url().'success''location'301); 
                    }  
            }
        protected function 
    submit_ad()
            {
                
    $this->load->model('members_model');//Insert this member
                
    $new_member_id $this->members_model->add_member();
                
    $this->load->model('ad_model');//Insert ad 
                
    $sucees_id $this->ad_model->insert_ad($new_member_id);
                
    $this->load->library('new_share/email_sender','','share_email');
                
    $this->share_email->index($sucees_id);
            }

    Last edited by zxcvbnm; December 4th, 2012 at 04:20 AM.
  22. #12
  23. Mad Scientist
    Devshed Expert (3500 - 3999 posts)

    Join Date
    Oct 2007
    Location
    North Yorkshire, UK
    Posts
    3,661
    Rep Power
    4124
    Originally Posted by zxcvbnm
    What did you study at uni Northie?
    Physics, here
    I said I didn't like ORM!!! <?php $this->model->update($this->request->resources[0])->set($this->request->getData())->getData('count'); ?>

    PDO vs mysql_* functions: Find a Migration Guide Here

    [ Xeneco - T'interweb Development ] - [ Are you a Help Vampire? ] - [ Read The manual! ] - [ W3 methods - GET, POST, etc ] - [ Web Design Hell ]

IMN logo majestic logo threadwatch logo seochat tools logo