6
\$\begingroup\$

I am currently developing a custom CMS being built on top of Codeigniter and was wondering if you can spot any flaws in my page fetching model code. The page fetching model is not entirely complete but the main functionality for retrieving a page is done, as well as retrieving modules assigned to a page (a module is really just a widget).

Can this model be better in some parts, perhaps in relation to the joins I am doing? although not really joins, but multiple queries to pull out bits of related info the pages like modules and media.

<?php

  class Mpages extends CI_Model {

      public function __construct()
      {
          parent::__construct();
      }

      public function fetch_page($page_slug = 'home') 
      {
          $db = $this->db;

          $query = 
            $db
               ->where('page_status', 1)
               ->where('page_slug', strtolower($page_slug))
               ->get('pages')
               ->result_array();

          $page_id = $query[0]['id'];

          $query['modules'] = 
            $db
                ->select('modules.module_name, modules.module_slug, modules.id moduleid')
                ->where('page_id', $page_id)
                ->join('pages_modules lpm', 'moduleid = lpm.module_id')
                ->order_by('module_order', 'asc')
                ->get('modules')
                ->result_array();

          /*$query['media'] = 
            $db
                ->select('lucifer_media.media_file_name, lucifer_media.media_file_extension, lucifer_media.media_directory')
                ->join('lucifer_pages_media', 'lucifer_pages_media.page_id = '.$page_id.'')
                ->get('lucifer_media')
                ->result_array();*/

          if ($query) {

            return $query;

          } else {
            return false;
          }
      }

      public function fetch_navigation()
      {
        $result = $this->db->order_by("nav_order", "asc")->where('published', 1)->get('navigation')->result_array();
        return $result;
      }

      public function fetch_layout($id)
      {

          $result = $this->db->where('id', $id)->get('layouts')->result_array();

          return $result[0];

      }

  }

?>
\$\endgroup\$
4
\$\begingroup\$

aaaah, a CodeIgniter fella :-)

I'm just working on a CI project myself and already implemented some of the optimization you could use for your CMS as well... so let's have a look:

  1. for as little overhead as possible, try implementing lazy-loading of your files (libraries, models...)

  2. for caching purposes, you can use KHCache - a library that allows you to cache parts of the website instead of full page

  3. instead of always doing $this->db->..., you can create a helper function, for instance "function _db()" and then simply do _db()->where...

  4. also, you can optionally create a helper function to give you the results array automatically, so ->result_array() will not be neccessary anymore: function res() {} ... $query = res(_db()->where...);


now, for the code :-)

$query = 
    $db
      ->where('page_status', 1)
      ->where('page_slug', strtolower($page_slug))
      ->get('pages')
      ->result_array();

$page_id = $query[0]['id'];

here, you seem to be selecting all values from DB, while in need of a single first ID - try limiting number of results or this will create overhead in your database

$db->where...->limit(1);

the second query could probably use a LEFT JOIN instead of a regular JOIN, although I leave it to you to decide (the JOIN approach might not list everything you need)

$db-select...->join('pages_modules lpm', 'moduleid = lpm.module_id', 'left')

I guess that's all... just remember to put correct indexes on your SQL fields and use the EXPLAIN statement to check for bottlenecks

good luck!

\$\endgroup\$
  • \$\begingroup\$ and since I can't post links in answers due to low rating, here they are - lazy load: codeigniter.com/forums/viewthread/134605 \$\endgroup\$ – Zathrus Writer Jan 27 '11 at 16:43
  • \$\begingroup\$ you can find KHCache here: codeigniter.com/forums/viewthread/69843 \$\endgroup\$ – Zathrus Writer Jan 27 '11 at 16:43
  • \$\begingroup\$ and at last, EXPLAIN statement from MySQL manual: dev.mysql.com/doc/refman/5.0/en/explain.html ... I'd be delighted if you accepted this answer if you found it useful, so I can get rid of these link limitations :-D \$\endgroup\$ – Zathrus Writer Jan 27 '11 at 16:45
  • \$\begingroup\$ instead of a helper function why not $db =& $this->db and then do $db->select()... \$\endgroup\$ – RobertPitt Jan 29 '11 at 12:15
  • \$\begingroup\$ Robert - to save time... _db()->select is quicker and requires only 1 line of code, while as with your solution, you need to write more of the same code every time (which could also change in the future, so you'd need to do extensive search/replaces in all files) \$\endgroup\$ – Zathrus Writer Jan 29 '11 at 13:34
4
\$\begingroup\$

I don't know Codeigniter so i can't comment on "is this a propper model". For my answer i'm just going to assume it is.

It's not much code so i'm going to focus on some details:


  public function __construct()
  {
      parent::__construct();
  }

Those 5 lines do absolutely nothing. I'd remove them. If you need a constructor later, add it later.


  public function fetch_navigation()
  {
    $result = $this->db->order_by("nav_order", "asc")->where('published', 1)->get('navigation')->result_array();
    return $result;
  }

This is a pretty hard to read (113 Chars in that line). I'd go for something like this (to say with your formatting from above)

  public function fetch_navigation()
  {
      return $this->db
          ->order_by("nav_order", "asc")
          ->where('published', 1)
          ->get('navigation')
          ->result_array();
  }

$db = $this->db;

Seems unnecessary to create a local variable, those 6 extra chars shoudn't hurt. I needed to look twice to see where that variable is from and if its local because changes are made to it.


So far those are really minor notes.

Unreachable code ?

if ($query) {
  return $query;
} else {
  return false;
}

you are setting $query['modules'] = ... so even if that is null $query will at least contain

array("modules" => null); 

and that will always be true.

\$\endgroup\$
  • \$\begingroup\$ I appreciate you taking the time and effort to post a solution. As for the constructor, you need that to interface with the parent controller of the Codeigniter framework, otherwise I do believe you can't use pre-loaded libraries, models and other things loaded automatically in the core. Valid point about $this->db though. I've always done that, but have also wondered if assigning it to a $db variable was worth the extra effort and convoluted things. Thanks for the modules null tip as well. \$\endgroup\$ – Dwayne Charrington Jan 20 '11 at 3:56
  • \$\begingroup\$ Erm, $db =& $this->db; would be more beneficial would it not? \$\endgroup\$ – RobertPitt Jan 20 '11 at 9:16
  • \$\begingroup\$ @RobertPitt Doing thats might be even harmful. Objects are passed "as reference" anyways and are hurtful php in general. There are only some very special cases where there is -any- benefit in using them \$\endgroup\$ – edorian Jan 20 '11 at 10:03
  • \$\begingroup\$ @Dwayne If you just leave the lines out the constructor of the parent class will be called by php. The only thing that you shoudn't to is leave it completely empty (thats also why i don't like "always having an constructor" someone might leave it blank and break stuff \$\endgroup\$ – edorian Jan 20 '11 at 10:05
  • 2
    \$\begingroup\$ @RobertPitt Juding from php.net/manual/en/language.oop5.references.php this happed with the PHP 5.0 release - I don't have access quick to an really old 5.1.6 to test it but i'm pretty sure :) If not please let me know \$\endgroup\$ – edorian Jan 20 '11 at 10:24

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service, privacy policy and cookie policy

Not the answer you're looking for? Browse other questions tagged or ask your own question.