[eluser]johnnytoobad[/eluser]
Hi there, I'm in the process of learning CI so I can start to implement it on the new sites I develop. I have a bunch of question but I thought rather than just post away on the forum, I'd have a go at making a simple app. So i've written a simple app to list, add, edit and delete records in a simple table. I was hoping if someone had a few minutes they would take a look at my effort and give me some critical advice on how I have coded it. There is a Model, Controller and two View files (plus some CSS). Its very basic and I'm CI newbie but I'm just looking for general advice, have i got the code split (MVC) right? Am I using too much code to achieve the basics? etc.
My files are here:
http://tinyurl.com/dnedfo
I then intend to update this simple app so i can use it as a reference as i scale my efforts upwards! Thanks in advance.
[eluser]TheFuzzy0ne[/eluser]
I've briefly commented on your controller. Hope this helps.
Code: <?php
class Owners extends Controller {
function Owners()
{
parent::Controller();
$this->load->helper('form');
$this->load->helper('url');
$this->load->database();
# These have already been loaded above, also, no need to load the form helper if you're loading the validation library.
#$this->load->helper(array('form', 'url'));
$this->load->library('form_validation');
$this->form_validation->set_error_delimiters('<span style="color:#FF0000;">', '</span>');
# No need to specify TRUE here unless your assigning to a variable.
#$this->load->model('Owner_model', '', TRUE);
$this->load->model('Owner_model');
}
function index()
{
# Don't use $_POST['search'], use $this->input->post('search'), then you won't need to use isset, either.
#if(isset($_POST['search'])) {
if ($searchText = $this->input->post('search')) {
# Don't use $_POST.
#$searchText = $_POST['search'];
//Get the records searched for
$this->db->like('fname', $searchText);
$this->db->or_like('lname', $searchText);
$this->db->or_like('company_name', $searchText);
$this->db->or_like('email', $searchText);
$this->db->or_like('username', $searchText);
$this->db->order_by("lname", "asc");
} else {
//Get all records
$this->db->order_by("lname", "asc");
}
//Execute the query
$query = $this->db->get('owners');
# Don't use $_POST.
#if(isset($_POST['search'])) { $data['searchText'] = $searchText; };
if($this->input->post('search')) { $data['searchText'] = $searchText; };
//Populate $data and load view
$data = array(
'num_rows' => $query->num_rows(),
'query' => $query,
'lname' => '',
'heading' => 'View all owners'
);
$this->load->view('owners_list_all', $data);
}
function owners_add()
{
# I assume this function is currently incomplete?
$data = array(
'owner_id' => '',
'fname' => '',
'lname' => '',
'telephone' => '',
'email' => '',
'company_name' => '',
'heading' => 'Add owner',
'form_path' => 'owners/owners_insert/'
);
$this->load->view('owners_form', $data);
}
function owners_insert()
{
# I'd suggest a little more validation. Check length and xss_clean
$this->form_validation->set_rules('fname', 'first name', 'required');
$this->form_validation->set_rules('lname', 'last name', 'required');
if ($this->form_validation->run() == FALSE) {
$data = array(
'owner_id' => '',
#'fname' => $_POST['fname'],
'fname' => $this->input->post('fname'),
#'lname' => $_POST['lname'],
'lname' => $this->input->post('lname'),
#'company_name' => $_POST['company_name'],
'company_name' => $this->input->post('company_name'),
#'email' => $_POST['email'],
'email' => $this->input->post('email'),
#'telephone' => $_POST['telephone'],
'telephone' => $this->input->post('telephone'),
'heading' => 'Add owner',
'form_path' => 'owners/owners_insert/'
);
$this->load->view('owners_form', $data);
} else {
# Your model accepts no parameters, yet it adds an owner.
$this->Owner_model->add_owner();
# The third parameter doesn't appear to get used by the index function. If you user is
# logged in, you might want to consider using flashdata to pass a message over, then you can omit the "/index/added"
redirect('/owners/index/added');
}
}
# function owners_edit()
function owners_edit($owner_id=FALSE)
{
# No need to use the segment directly. You can add the variable to the function declaration
# and it will be passed in automatically.
#if ($this->uri->segment(3) === FALSE) {
if ( ! is_numeric($owner_id)) { # added a little validation, assuming the id is numeric.
redirect('/owners/');
} else {
#$this->db->where('owner_id', $this->uri->segment(3));
$this->db->where('owner_id', $owner_id);
//Execute the query
$query = $this->db->get('owners');
$row = $query->row();
#What happens if no rows are returned?
$data = array(
# 'owner_id' => $this->uri->segment(3)
'owner_id' => $owner_id,
'fname' => $row->fname,
'lname' => $row->lname,
'telephone' => $row->telephone,
'email' => $row->email,
'company_name' => $row->company_name,
'heading' => 'Edit owner',
'form_path' => 'owners/owners_update/',
);
$this->load->view('owners_form', $data);
}
}
[eluser]TheFuzzy0ne[/eluser]
Code: function owners_update()
{
# Again, I'd suggest a little more validation here.
$this->form_validation->set_rules('fname', 'first name', 'required');
$this->form_validation->set_rules('lname', 'last name', 'required');
if ($this->form_validation->run() == FALSE) {
# Ah-ha! I see you changed your style here. Good for you! :)
$data = array(
'owner_id' => '',
'fname' => $this->input->post('fname'),
'lname' => $this->input->post('lname'),
'company_name' => $this->input->post('company_name'),
'email' => $this->input->post('email'),
'telephone' => $this->input->post('telephone'),
'heading' => 'Edit owner',
'form_path' => 'owners/owners_update/'
);
$this->load->view('owners_form', $data);
} else {
# Again, no paramters specified to your model method.
$this->Owner_model->update_owner();
# The third parameter doesn't appear to get used by the index function. If you user is
# logged in, you might want to consider using flashdata to pass a message over, then you can omit the "/index/edited"
redirect('/owners/index/edited/');
}
}
function owners_delete($id=FALSE)
# function owners_delete()
{
# Direct call to database in your controller. This should be in a model.
$this->db->delete('owners', array('owner_id' => $id));
# The third parameter doesn't appear to get used by the index function. If you user is
# logged in, you might want to consider using flashdata to pass a message over, then you can omit the "/index/deleted"
redirect('/owners/index/deleted/');
}
}
?>
[eluser]jedd[/eluser]
And my 2c worth.
Think in terms of an instance of an object (person, place, etc) - and don't use plurals on your Class names. It'll confuse you later. Same logic as applies to avoiding plurals on table and column names in your schema.
Your methods in this function also include the string 'owners_' - which means your (native) URL's would look like:
http://host.org/owners/owners_add/
If you just have methods like add, edit, view, etc in your Class, then your url would look like this:
http://host.org/owner/add/
I'm a big proponent of using data's parentage to give it context, rather than replicating descriptors all the way down.
[eluser]johnnytoobad[/eluser]
Thanks very much for all your replys. Really useful, I have a couple of questions about your comments:
Code: # Your model accepts no paramters, yes it adds an owner.
$this->Owner_model->add_owner();
# The third parameter doesn't appear to get used by the index function. If you user is
# logged in, you might want to consider using flashdata to pass a message over, then you can omit the "/index/added"
redirect('/owners/index/added');
The two things here are i'm not quite sure what you mean buy my model accepts no parameters. I though i was just calling the function in the model.
Secondly the reason i have the extra /added is to pass the variable through to the index page to display a box saying the record was added/deleted/updated:
Code: <? if($this->uri->segment(3)) { ?>
<p class="infoBox">The owner was successfully <?=$this->uri->segment(3)?></p>
<? } ?>
Is there a better way of doing this?
Thanks again for the great replies.
[eluser]jedd[/eluser]
Quote:The two things here are i'm not quite sure what you mean buy my model accepts no parameters. I though i was just calling the function in the model.
I read that as 'if you are going to have a function called 'add', you probably want to identify what you're adding'.
Quote:Secondly the reason i have the extra /added is to pass the variable through to the index page to display a box saying the record was added/deleted/updated
While this is a neat approach, I'd be wary about this. It assumes that you will always have a word there that parses well into a given sentence, and you might forget this requirement in the future when adding/modifying functions or changing the preamble of that sentence.
More importantly I'd be cautious of relying on uri segment numbers to never change (say if you ever re-structure your project - add directories, etc). It seems gratuitous, given the minimal code it's saving.
[eluser]johnnytoobad[/eluser]
How can I pass an variable through the redirect so I can use it in the function to control the display of the feedback box?
[eluser]TheFuzzy0ne[/eluser]
[url="http://ellislab.com/codeigniter/user-guide/libraries/sessions.html"]Flashdata[/url].
[eluser]johnnytoobad[/eluser]
Great thanks for all your (speedy :-) ) help, it was exactly what I needed. Now onward and upward...
[eluser]TheFuzzy0ne[/eluser]
Sorry for the short reply. I am in a rush to get some food, and wanted to try and reply to some posts first.
Good luck with your project. 
|
