CodeIgniter Forums
Help me become a better developer! - Printable Version

+- CodeIgniter Forums (https://forum.codeigniter.com)
+-- Forum: Archived Discussions (https://forum.codeigniter.com/forumdisplay.php?fid=20)
+--- Forum: Archived Development & Programming (https://forum.codeigniter.com/forumdisplay.php?fid=23)
+--- Thread: Help me become a better developer! (/showthread.php?tid=2155)

Help me become a better developer! - El Forum - 07-18-2007

[eluser]awpti[/eluser]
So, I'm now on month 1 of CodeIgniter. Wow.

I'll post my Model and Controller (Only have one of each, for now) here.

What I'm specifically looking for:

Comments.

Be blunt, I can take it.

Tell me I'm doing something completely stupid if I am, in fact, doing something completely stupid.

I currently use Coolfactor's awesome View library.

So, without further adeu:

controller/blog.php

Code:
class Blog extends Controller {

private $awTitle = 'awpti.org - Where geeks roam free!';
private $awHeader = 'awpti.org';

function Blog() {
  parent::Controller();

  $this->load->model('blogmodel', 'blog', TRUE);
  
  //Set the header/footer template
  $this->view->part('header', 'common/header.php');
  $this->view->part('footer', 'common/footer.php');
}

function index() {
  $this->view->set('Title', $this->awTitle);
  $this->view->set('Header', $this->awHeader); //need to rename this variable. maybe later

  $this->view->set('Query_News', $this->blog->get_latest_news(3));;
  $this->view->load('blog/front');
}

function comments() {
  $this->view->set('Title', $this->awTitle.' (Comments)');
  $this->view->set('Header', $this->awHeader);

  if( !$this->uri->segment(2) || !ctype_digit($this->uri->segment(2)) ) {
   $this->view->set('Query_News',  $this->blog->get_latest_news(1));
   $this->view->set('Query_Comments', $this->blog->get_comments());
  } else {
   $this->view->set('Query_News', $this->blog->get_news_by_id( $this->uri->segment(2) ));
   $this->view->set('Query_Comments', $this->blog->get_comments( $this->uri->segment(2) ));
  }
  
  $this->view->set('CommentID', $this->uri->segment(2));
  
  $this->view->load('blog/comments');
}

/*

Code snipped for brevity, nothing to improve here as they are placeholders/static content

*/

} //end class. Duh.

And..

models/blogmodel.php

Code:
class Blogmodel extends Model {

function Blogmodel() {
  parent::Model();
}

function get_latest_news($limit) {
  $news = $this->db->query('SELECT *,
      IF( awNewsComments.comments_news_id IS NOT NULL, count( * ), 0 ) AS num_comments
      FROM awNews
      LEFT JOIN awNewsComments ON awNews.news_id = awNewsComments.comments_news_id
      GROUP BY awNews.news_id ORDER BY awNews.news_id DESC
      LIMIT '.$limit);
  return $news->result();
}

function get_comments($news_id = '') {
  if(!$news_id) {
   $query_comments = $this->db->query("SELECT * FROM awNewsComments WHERE comments_news_id = (SELECT MAX(news_id) FROM awNews) ORDER BY comments_id DESC");
  } else {
   $query_comments = $this->db->query("SELECT * FROM awNewsComments WHERE comments_news_id = {$news_id} ORDER BY comments_id DESC");
  }

  return $query_comments->result();
}

function get_news_by_id($news_id) {
  $news = $this->db->query('SELECT * FROM awNews WHERE news_id = '.$news_id);
  return $news->result();
}
}

I attempted to avoid any sort of logic in the models, but I couldn't devise any way to do it without creating a bunch of fluff, redundant functions.

To recap: Blast me as hard as you can, but throw some lessons in there, if possible.

Help me become a better developer! - El Forum - 07-18-2007

[eluser]batteries[/eluser]
"blast me as hard as you can".. haha, i like.

ok. fine.

how sure are you that your $limit variable is clean? cast to an integer you damn n00b!! like so.

Code:
$sql = 'blha lbha LIMIT '.(int)$limit;

also, setting your sql statements to variable will make things a little neater:

Code:
$sql = 'SELECT * FROM WHERE x = 4 ';
$sql.= 'AND this = $that';

$query = $this->db->query($sql);

also, i think it's better to check your db query results in the model. returning results on TRUE and error codes/FALSE on no result. but that's just me. this keeps the controller cleaner.

Edit,just incase: .. only cast to (int), values that numerical!

Help me become a better developer! - El Forum - 07-18-2007

[eluser]Rick Jolly[/eluser]
Two things come to mind:

1. escape your sql
Code:
// you could let CI escape your sql automatically by using query bindings:
$sql = "SELECT * FROM awNewsComments WHERE comments_news_id = ? ORDER BY comments_id DESC";
$this->db->query($sql, array($news_id));

2. beware of sql injection. For example, you could verify that $news_id is a number (and therefore you would know $news_id isn't some malicious sql). EDIT: Ah, you checked for a number, sorry I missed it the first time.

Help me become a better developer! - El Forum - 07-18-2007

[eluser]Rick Jolly[/eluser]
One more thing - I see you are using php 5 based on your private properties. You could use the php 5 constructor for the sake of consistency:
Code:
function __construct() {
  parent::Controller();
}
Note though that since CI is php 4 you can't call the parent constructor like this: parent::__construct();

Also, I'm not sure if you are aware but you don't need the constructor in the model since you aren't using it for anything. It doesn't hurt to have it though.

Help me become a better developer! - El Forum - 07-18-2007

[eluser]awpti[/eluser]
Thanks for the replies, gents!

I'm slowly working on the sql injections holes - mainly just checking if ctype_digit() against ID values.

Code:
if( !$this->uri->segment(2) || !ctype_digit($this->uri->segment(2)) ) {

If that little number fails, it just defaults to pulling the newest post and showing related comments.

As far as $limit - I always know that's going to be a numerical value. It's only ever passed internally.

( get_latest_news(3) )
( get_latest_news(1) )

I'll give that SQL Binding thing a try.

Help me become a better developer! - El Forum - 07-19-2007

[eluser]Rick Jolly[/eluser]
Another issue - xss. See the comments below:
Code:
function comments() {
  $this->view->set('Title', $this->awTitle.' (Comments)');
  $this->view->set('Header', $this->awHeader);

  if( !$this->uri->segment(2) || !ctype_digit($this->uri->segment(2)) ) {
   $this->view->set('Query_News',  $this->blog->get_latest_news(1));
   $this->view->set('Query_Comments', $this->blog->get_comments());
  } else {
   $this->view->set('Query_News', $this->blog->get_news_by_id( $this->uri->segment(2) ));
   $this->view->set('Query_Comments', $this->blog->get_comments( $this->uri->segment(2) ));
  }
  
  // This could be a problem since you are assigning potentially unclean data to the view.
  // You should move this into your "else" above.
  $this->view->set('CommentID', $this->uri->segment(2));
  
  $this->view->load('blog/comments');
}


Help me become a better developer! - El Forum - 07-19-2007

[eluser]awpti[/eluser]
I concur!

Didn't even think about that one. That's going to be part of my comment system - though most likely I'll do some hardcore sanity checking once I bring the comments to life. If ever (I can't get the forms looking good).

Help me become a better developer! - El Forum - 07-19-2007

[eluser]RaZoR LeGaCy[/eluser]
How does this measure up??

Code:
if ( !$this->uri->segment(3) || !ctype_digit($this->uri->segment(3)) ){

redirect('movies/');

} else {

$this->load->helper('text');

#$this->output->cache(0);
#$this->output->enable_profiler(TRUE);

#Queries

$this->db->where('rid', $this->uri->segment(3));
$data['reviews'] = $this->db->get('hellhorror_reviews');

}