Page 1 of 1

Need help optimizing code and need input on this design

Posted: Thu May 29, 2008 8:22 am
by FrostbyteX
Below are three files in a small CMS I'm writing. list.php and create.php are views for listing and creating content. content.class.php contains the definition for the Content class, which represents a single row in the "content" table. Records are retrieved, created and updated through member variables and functions in this content class; the idea behind this is to reduce complexity for accessing content, and to better fit an object oriented design (MVC is where I'm leaning at the moment).

I'm new to PHP, so I've probably made many beginner mistakes. Please point them out. That's why I'm here.

Also, what are your thoughts on having classes "wrap" each table to make access simpler and upgrading easier?

Thanks,
Steve

list.php

Code: Select all

<?php
 
require_once('../classes/content.class.php');
 
$contentList = Content::FindAll();
?>
 
<html>
  <head>
    <title>List Content</title>
  </head>
  <style>
  ol.list_roman
  {
    list-style-type: arabic-numbers;
  }
  
  div#actions
  {
    padding: 0.5em; 
    margin: 0.5em; 
    border: 2px solid black;
    display: inline;
  }
  </style>
  <body>
    <h1>Content</h1>
    <h2>List</h2>
    <div id="actions"><a href="new.php">New Content</a></div>
    <div>
    <ol class="list_roman">
    <?php 
    foreach($contentList as $content)
    {
        echo "<li>" .
             "<a href=\"view.php?id=".$content->id()."\">" . $content->title . 
             "</a> - <a href=\"remove.php?id=".$content->id()."\">Delete</a></li>";
    }
    ?>
    </ol>
    </div>
    <a href="../index.php">Back</a>
  </body>
</html>
create.php

Code: Select all

<?php 
 
require_once('../classes/content.class.php');
 
?>
 
<html>
<body>
<h1>Content</h1>
<h2>Create</h2>
<div>
<?php 
$title = filter_input(INPUT_POST, 'title', FILTER_SANITIZE_MAGIC_QUOTES);
$contents = filter_input(INPUT_POST, 'contents', FILTER_SANITIZE_MAGIC_QUOTES);
 
if(Content::Create($title, $contents) !== false)
{
    echo "Content created";
}
else
{
    echo "Error creating content: ".$db->Error();
}
 
?>
</div>
<a href="list.php">Back</a>
</body>
</html>
content.class.php

Code: Select all

<?php 
 
require_once('../db/initialize.php');
 
class Content
{
    private static $table_name = "content";
    
    private $id;
    public $parent_id;
    public $user_id;
    public $title;
    public $contents;
    public $timestamp;
    public $type;
    public $date_created;
    
    public function id()
    {
        return $this->id;
    }
    
    public static function Create($title, $contents, $user_id = 0, $type = 0, $parent_id = 0)
    {
        global $db;
        if(!$db->Query("INSERT INTO ".self::$table_name.= " (title, contents, user_id, 
                                                         type, parent_id, date_created) 
                    VALUES ('$title', '$contents', $user_id, $type, $parent_id, NOW())"))
            return false;
        return new Content($db->InsertId(), 
                           $parent_id, 
                           $user_id, 
                           $title, 
                           $contents, 
                           date("Y-m-d G:i:s"), 
                           $type, 
                           date("Y-m-d G:i:s"));
        
    }
    
    public static function Remove($pattern)
    {
        global $db;
        
        return $db->Query("DELETE FROM ".self::$table_name.= " WHERE ".$pattern);
    }
    
    public static function RemoveById($id)
    {
        return self::Remove("id = ".$id);
    }
    
    private function Content($id, $parent_id, $user_id, $title, $contents, $timestamp, $type, $date_created)
    {
        $this->id = $id;
        $this->parent_id = $parent_id;
        $this->user_id = $user_id;
        $this->title = $title;
        $this->contents = $contents;
        $this->timestamp = $timestamp;
        $this->type = $type;
        $this->date_created = $date_created;
    }
    
    public static function Find($pattern = "", $flags = array())
    {
        global $db;
        
        $retval = array();
        $sql = "SELECT id, parent_id, user_id, title, contents, timestamp, type, date_created".
                    " FROM ".self::$table_name;
        
        if($pattern != "")
        {
            $sql .= " WHERE ".$pattern;
        }
        
        $result = $db->Query($sql);
        if($result)
        {
            while($row = mysql_fetch_assoc($result))
            {
                $content = new Content($row["id"], 
                                       $row["parent_id"], 
                                       $row["user_id"], 
                                       $row["title"], 
                                       $row["contents"], 
                                       $row["timestamp"], 
                                       $row["type"], 
                                       $row["date_created"]);
                
                if(array_search("first", $flags) !== false)
                {
                    return $content;
                }
                else
                {
                    array_push($retval, $content);
                }
            }
        }
        
        return $retval;
    }
    
    public static function FindById($id)
    {
        return self::Find("id = ".$id, array("first"));
    }
    
    public static function FindAll()
    {
        return self::Find();
    }
}
 
?>

Re: Need help optimizing code and need input on this design

Posted: Sun Jun 08, 2008 9:53 pm
by Attilitus
This is general design advice.

1) You should not have ANY php in your templates/views. You ought to have a single template parsing function. (It works by fetching the contents of the template and then replacing variables within with values that are passed to the function, usually in an array).

2) You should have a single DB abstraction class that automatically escapes data. All other functions that handle data ought to interface with this common "safe" db abstraction class.

3)You ought to rethink your coding pattern. The current pattern is too complex for accomplishing something that is very very simple. However, I know that every coder is a little bit "in love" with their own way of doing things, so I'm probably baised.