Need help optimizing code and need input on this design

Coding Critique is the place to post source code for peer review by other members of DevNetwork. Any kind of code can be posted. Code posted does not have to be limited to PHP. All members are invited to contribute constructive criticism with the goal of improving the code. Posted code should include some background information about it and what areas you specifically would like help with.

Popular code excerpts may be moved to "Code Snippets" by the moderators.

Moderator: General Moderators

Post Reply
FrostbyteX
Forum Newbie
Posts: 3
Joined: Wed May 28, 2008 3:19 pm

Need help optimizing code and need input on this design

Post 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();
    }
}
 
?>
Attilitus
Forum Commoner
Posts: 27
Joined: Wed Aug 08, 2007 2:32 pm

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

Post 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.
Post Reply