• 0 Vote(s) - 0 Average
  • 1
  • 2
  • 3
  • 4
  • 5
Help me become a better developer!

#1
[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.

#2
[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!

#3
[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.

#4
[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.

#5
[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.

#6
[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');
}

#7
[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).

#8
[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');

}


Digg   Delicious   Reddit   Facebook   Twitter   StumbleUpon  


  Theme © 2014 iAndrew  
Powered By MyBB, © 2002-2021 MyBB Group.