Critique

[eluser]onethirtyone[/eluser]
Hello Everyone,

My name is Bill and I am a bit new to codeigniter, I just wanted to drop in and say hello, I also wanted to post some of my code for what is now a simple CRUD app to see if you guys could give me feedback and let me know what I am doing right, and what I could improve upon. So far all it does is iterate some student names from a db and let the user delete them. Any advice would be appreciated a bunch! I am still new to PHP and especially codeigniter. All of the views are labeled as fucntion_content because I have a standard wrapper called main view which has a toolbar and a header, I am not going to bother posting that because there is really no php in it except for variable calls for links and title info. Basically Main_View calls a php include in order to spit the desired content out in the body of the website. Also, I have been passing variables using URI's I figure sessions or cookies are a better idea, but i havent learned how to use them yet, thoughts?


Students Controller

Code:

<?

class Students extends Controller
{
    function Students ()
    {
    // load controller parent
    parent::Controller();
    // load 'students_model' model
    $this->load->model('Students_model', 'Students');
    }
    
    
    function index()
    {
    $this->load->helper('url');
    $this->load->helper('form');
    $this->load->helper('asset');
    // data for page configuration
    $data['title'] = "CCS Student Management";
    $data['stylesheet'] = css_asset('main_view.css');
    $data['content'] = "students_content.php";
    // data from sql queries
    $data['students']=$this->Students->getAllStudents();
    $data['numstudents']=$this->Students->getNumStudents();
    // load 'main_view' view
    $this->load->view('main_view', $data);
    }
    
    function delete()
    {
    $this->load->helper('url');
    $this->load->helper('form');
    $this->load->helper('asset');
    // data for page configuration
    $data['id'] = $this->uri->segment(3);
    $data['students'] = $this->Students->getStudentsWhere('id',$data['id']);
    $data['title'] = "Confrim Student Deletion";
    $data['stylesheet'] = css_asset('main_view.css');
    $data['content'] = "delete_content.php";
    // load 'main_view' view
    $this->load->view('main_view', $data);
    }

    function confirm_delete()
    {
    $this->load->helper('url');
    $id = $this->uri->segment(3);
    $confirm = $this->input->post('confirm');
        if($confirm=="Yes")
        {
        $this->Students->deleteStudentsWhere('id',$id);
        }
     redirect('/students/', 'location');
    }
}
?>

Students Model

Code:

<?

class Students_model extends Model
{
    function Students_model()
    {
    // load the Model parent
    parent::Model();
    }

    function getAllStudents()
    {
    // sort by student then select them all
    $this->db->order_by("student", "asc");    
    $query=$this->db->get('Students');
    if($query->num_rows()>0)
    {
    // return result set as an associative array
    return $query->result_array();
    }
    }

    function getStudentsWhere($field,$param)
    {
    $this->db->where($field,$param);
    $query=$this->db->get('Students');
    // return result set as an associative array
    return $query->result_array();
    }
    
    // get total number of users
    function getNumStudents()
    {
    return $this->db->count_all('Students');
    }

    //delete students
    function deleteStudentsWhere($field,$param)
    {
    $this->db->delete('Students', array($field => $param));
    }
}
?>

Students View - Needs Styling lol

Code:

<table class="studentlist">
    <tr>
       <td><h4>ID</h4></td>
       <td><h4>Student Name</h4></td>
    </tr>
&lt;? foreach($students as $student):?&gt;
    <tr>
       <td>&lt;?=$student['ID'];?&gt;</td>
       <td>&lt;?=$student['student'];?&gt;</td>
    
       <td>
       <a href="&lt;?=site_url(">Delete</a>
       </td>
    </tr>
&lt;? endforeach; ?&gt;
    <tr>
       <td colspan="3">Total Students: &lt;?=$numstudents;?&gt;</td>
    </tr>    
</table>

Delete Confirmation - Just a yes no page to prevent accidental deletion

Code:

<h3> Confirm Deletion of Student </h3>
<p> Are you sure you wish to Delete &lt;?foreach($students as $student){echo $student['student'];}?&gt;?</p>
&lt;form action="&lt;?=site_url("/students/confirm_delete/");?&gt;/&lt;?=$id;?&gt;" method="post"&gt;
<p>
&lt;input type="submit" name="confirm" value="No" class="submit" /&gt;
&lt;input type="submit" name="confirm" value="Yes" class="submit" /&gt;
</p>
&lt;/form&gt;

[eluser]Colin Williams[/eluser]
Looks pretty good. My only immediate critique is that the coding style is a bit sporadic and verbose.

Something like $this->Students->deleteStudentsWhere() could simply be $this->students->delete(), and so on. I would also reference the style guide provided in the user guide for more suggestions.

Also don't see the need for a getAllStudents and getStudentsWhere. You could simply have get() that gets all students if there are no supplied arguments, get a student by id if an int is passed, get students based on conditions if an associative array is passed, etc.

[eluser]JoostV[/eluser]
Welcome, onethirtyone.

Looking good. A bit verbose/repetitive sometimes. Some ponderings:
* You could gather all repetitive code in a private function (or move it to the model)

Code:

function _loadLibs() {
    $this->load->helper('url');
    $this->load->helper('form');
    $this->load->helper('asset');
    $data['stylesheet'] = css_asset('main_view.css');
    // (...)
    return $data;
}

* It would be wise to filter $id before deletion, imho. If you pass an empty value the active record library will delete all records in your table. (filter input escape output)

Code:

function deleteStudents($id)
{
    $id = (int) $id;
    if ($id > 0) {
        $this->db->limit(1);
        $this->db->delete('Students', array('id' => $id));
    }
}

* You could create a more generic base model that features some basic CRUD actions like get($id), getWhere($array), delete($id), etc. This would be a base model that you can extend other models from. Saves you a lot of coding in child models later on. Just set some basic variables like $_tableName and $_primaryField that you can use in the base model functions. The delete function in such a base model could be something like: (off the top of my head)

Code:

function delete($id)
{
    $id = (int) $id;
    if ($id > 0) {
        $this->db->limit(1);
        if ($this->db->delete($this->_tableName, array($this->_primaryField => $id))) {
            // record was succesfully deleted
            return true;
        }
    }
    return false;
}

That's all for now. I'm off to bed. Happy coding!

[eluser]Dam1an[/eluser]
Welcome to CI
Just a few comments (I'll try not to repeat what has already been said too much)

You seem to load the URL helper in each controller method, so you could move that load into the constructor (although you could move all of them there, or have the common stuff in a seperate function as Joost said)

The getNumStudents method seems redundant... Its the same effect as calling sizeof on the strudents you've already got at that point

Instead of using uri->segment(3), I would pass it in as a parameter to the function (segments 3+ become parameters)

In your getStudentsWhere function you don't have a check before returning the results, this can cause problems if there are no matches, I always do

Code:

return $query->num_rows() > 0 ? $query->result() : false;

This appears to be a common thing by people on these forums

As has been said, I would put some checks in place on the delete function
Also maybe set a default limit (as a parameter) of 1

In your view, the delete seems to be broken

Code:

// Yours
<a href="&lt;?=site_url(">Delete</a>

// I recommend
&lt;?=anchor('students/delete/'.$user['id'], 'Delete')?&gt; // Notice that the user id will become the parameter

Thats it for now

Wait, one more thing, I know you said you need to style your view, but your table headings are wrong

Code:

// Yours
<table class="studentlist">
    <tr>
       <td><h4>ID</h4></td>
       <td><h4>Student Name</h4></td>
    </tr>

// Mine, using table headings instead of table data with heading tags thrown in
<table class="studentlist">
    <tr>
       <th>ID</th>
       <th>Student Name</th>
    </tr>

You can then style them as you want in the CSS (just makes it a little cleaner, and valid)

[eluser]onethirtyone[/eluser]

In your view, the delete seems to be broken

I noticed that too, looks like I botched copy/paste somehow.... some programmer I am.

I had used <th> before and I was having problems with the styling on my page, I think I changed it to td h4 as a troubleshooting measure and never fixed it, thanks for reminding me. I will also try to tidy up my code and not be so redundant when I can just load the common helper methods in the constructor and I will def also put some checks in my DB calls as well. Can you give me a code example of passing the ID as a param? is it just the " , Delete " in the URL? how do I reference this in the function?

Thanks for all the advice!!

-Bill