Critique My Codeigniter Custom CMS Pages Model

Question

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];

      }

  }

?>

Answer 1

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!

Answer 2

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.