The Shed is going Social! Join us on FaceBook and Twitter and chime in on the conversation.
|
 |
|
Dev Shed Forums
> Programming Languages
> PHP Development
|
Tips required to organize class...
Discuss Tips required to organize class... in the PHP Development forum on Dev Shed.
Tips required to organize class... PHP Development forum discussing coding practices, tips on PHP, and other PHP-related topics. PHP is an open source scripting language that has taken the web development industry by storm.
|
|
 |
|
|
|
|
Dev Shed Forums Sponsor:
|
|
|
December 2nd, 2012, 12:57 AM
|
 |
A Change of Season
|
|
|
|
|
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<0 || $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<0 || $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']<1 || $date_format['1']>12)
{
$this->invalid_month=true;
}
if(!is_numeric($date_format['2']) || $date_format['2']<1 || $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');
}
}
|
December 2nd, 2012, 03:19 PM
|
 |
Square Peg in a Round Hole
|
|
Join Date: Oct 2007
Location: North Yorkshire, UK
|
|
|
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?
|
December 2nd, 2012, 11:21 PM
|
|
A Change of Season
|
|
|
|
Quote: | 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.
|
December 3rd, 2012, 12:23 AM
|
|
Contributing User
|
|
Join Date: Dec 2012
Location: Ithaca
Posts: 64
Time spent in forums: 13 h 7 m 48 sec
Reputation Power: 1
|
|
|
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.
|
December 3rd, 2012, 03:13 AM
|
|
Square Peg in a Round Hole
|
|
Join Date: Oct 2007
Location: North Yorkshire, UK
|
|
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.
|
December 3rd, 2012, 05:24 PM
|
|
Contributing User
|
|
Join Date: Dec 2012
Location: Ithaca
Posts: 64
Time spent in forums: 13 h 7 m 48 sec
Reputation Power: 1
|
|
|
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.
|
December 3rd, 2012, 06:14 PM
|
|
A Change of Season
|
|
|
|
Quote: | 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: Quote:
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
|
|
December 3rd, 2012, 06:28 PM
|
|
Square Peg in a Round Hole
|
|
Join Date: Oct 2007
Location: North Yorkshire, UK
|
|
|
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
|
December 3rd, 2012, 07:52 PM
|
|
A Change of Season
|
|
|
|
Quote: | 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!!!
|
December 4th, 2012, 03:11 AM
|
|
Square Peg in a Round Hole
|
|
Join Date: Oct 2007
Location: North Yorkshire, UK
|
|
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,
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
|
December 4th, 2012, 03:46 AM
|
|
A Change of Season
|
|
|
|
Quote: | 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.
|
December 4th, 2012, 05:10 AM
|
|
Square Peg in a Round Hole
|
|
Join Date: Oct 2007
Location: North Yorkshire, UK
|
|
Quote: | Originally Posted by zxcvbnm What did you study at uni Northie? |
Physics, here
|
Developer Shed Advertisers and Affiliates
| Thread Tools |
Search this Thread |
|
|
|
| Display Modes |
Rate This Thread |
 Linear Mode
|
|
Posting Rules
|
You may not post new threads
You may not post replies
You may not post attachments
You may not edit your posts
HTML code is Off
|
|
|
|
|
Del.icio.us
Digg
Google
Spurl
Blink
Furl
Simpy
Y! MyWeb
