Page 1 of 1

critique my php5 template class

Posted: Sun Feb 04, 2007 7:28 am
by myUserName
I just made an updated\new version of my template class and would like to get it rated\reviewed. Any comments?

abstract

Code: Select all

<?php
abstract class Template_Abstract
{
	private $_data = array();
	private $_option = array('dir' => '');
	
	public function __construct()
	{
		$this->_config();
	}	
		
	public function assign($name, $value)
	{
		$this->_data[$name] = $value;
		
		return $this;
	}
	
	public function display($template, $buffer = true)
	{
		extract($this->_data);
		
		if ($buffer)
			ob_start();
			
		$path = $this->getPath($template);
		
		if (!file_exists($path))
			throw new Template_Exception(3);
		else 
			require $path;
			
		if ($buffer)
			return ob_get_clean();
			
		return true;
	}
	
	protected function setDir($dir)
	{
		if (!file_exists($dir) && $dir)
			throw new Template_Exception(2);
			
		$this->_option['dir'] = $dir;
	}
	
	protected function __get($name)
	{
		if ($this->__isset($name))
			return $this->_data[$name];
		else 
			throw new Template_Exception(4);
	}
	
	protected function __isset($name)
	{
		if (isset($this->_data[$name]))
			return true;
	}
	
	protected function __unset($name)
	{
		if ($this->__isset($name))
		{
			unset($this->_data[$name]);
			return true;
		}
	}
	
	protected function __set($name, $value)
	{
		return $this->_data[$name] = $value;
	}
	
	protected function _config()
	{
		
	}
	
	protected function _debug($data)
	{
		return $data;
	}
	
	protected function _debugDisplay($data)
	{
		echo "<pre>\n";
		print_r($data);
		echo "</pre>";
	}
	
	protected function debug()
	{
		$this->_debugDisplay($this->_debug($this->_data));
	}
	
	protected function getDir()
	{			
		return $this->_option['dir']; 
	}
	
	protected function getPath($file)
	{
		if ($file[strlen($file) - 1] == '/') 
		{
			$dir = $dir = $this->getDir();
		}
		else
		{
			if (strlen($this->getDir()) != 0)
				$dir = $this->getDir() . '/';
			else 
				$dir = '';
		}
		
		return  $dir . $file;
	}
	
	public function clear()
	{
		$this->_data = array();
	}
}
?>
exception

Code: Select all

<?php
class Template_Exception extends Exception 	
{
	public function __construct($code)
	{
		switch ($code):
			case 1:
				$message = 'template directory is not set';
				break;
			case 2:
				$message = 'template directory does not exist';
				break;
			case 3:
				$message = 'tempalte file does not exist';
				break;
			case 4:
				$message = 'that template variable has not been set';
				break;
			default:
				$message = 'unknown template exception';
				break;
		endswitch;
		
		parent::__construct($message, $code);
	}
}
?>
template

Code: Select all

<?php
class Template extends Template_Abstract
{
	public function _config()
	{
		$this->setDir($_SERVER['DOCUMENT_ROOT'] . '/' . 'templates');
	}
}
?>
factory

Code: Select all

<?php
function template()
{
	static $template = false;
	
	if ($template == false)
		$template = new Template();
	else
		$template->clear();
		
	return $template;
}
?>
in use

Code: Select all

echo template()->assign('one', 'two')
                        ->assign('three', 'four')
	  		->display('some_file.tpl');
or

Code: Select all

template()->assign('one', 'two')
                ->assign('three', 'four')
	  	->display('some_file.tpl', false);

Posted: Sun Feb 04, 2007 11:31 am
by Ollie Saunders
Reinventing the wheelness aside this is pretty nice. My main issue would probably be the lack of unit tests, lack of comment and lack of bracing on your control statements. I like the Zend Coding Standard (see sig) you see.

I don't like this at all...

Code: Select all

throw new Template_Exception(1);
...the readability of this is poor. I've got no idea what 1 means here without referring to the class definition. Also the stuff in Template_Exception::__construct() is a bit superfluous. Can't you just use the actual message where you need to throw? If you are concerned about duplication of messages you can make private functions in Template_Abstract that throw with specific messages. You'll also find that in PHP 5.1 this...

Code: Select all

class Template_Exception extends Exception      
{
        public function __construct($code)
...incurs an E_STRICT because the constructor prototype is being changed by inheritance reducing its polymorphism. Make sure you are developing with these set...

Code: Select all

ini_set('display_errors', true);
error_reporting(E_ALL | E_STRICT);
Here...

Code: Select all

protected function _debug($data)
        {
                return $data;
        }
       
        protected function _debugDisplay($data)
        {
                echo "<pre>\n";
                print_r($data);
                echo "</pre>";
        }
       
        protected function debug()
        {
                $this->_debugDisplay($this->_debug($this->_data));
        }
..._debugDisplay() breaks the single responsibility principle you should have a separate class or external function or doing such things; it is not the domain of Template_Abstract to format arrays for debugging purposes. Also what is the point of _debug() it just returns its param and nothing more?

Why not have a function getData() that returns the data, this can be used for more than just debugging, and an external debug function that does the stuff with the pre.

Code: Select all

debug($instance->getData());
if you are really lazy you can have one specifically for debugging templates like this...

Code: Select all

debugTemplate(Template_Abstract $template) {
    echo '<pre>', print_r($template->getData(), true), '</pre>';
}
debugTemplate($instance);
Finally how about you get rid of the abstractness and introduction a public method called setPath(). You could move some of the processing from here...

Code: Select all

protected function getPath($file)
        {
                if ($file[strlen($file) - 1] == '/')
                {
                        $dir = $dir = $this->getDir(); // < huh?
                }
                else
                {
                        if (strlen($this->getDir()) != 0)
                                $dir = $this->getDir() . '/';
                        else
                                $dir = '';
                }
               
                return  $dir . $file;
        }
...into it. Then you won't need to create a new class every time you want to change the directory for your templates and you can change the directory at runtime should you need to. Also I'm not sure why you are grouping stuff in Template_Abstract under $_option, why not just have a $_dir property.

OK I'm done.

Posted: Sun Feb 04, 2007 12:09 pm
by feyd
Can you edit the title to be a tad bit more specific than "critique this"?

Posted: Mon Feb 05, 2007 4:46 pm
by myUserName
can I change it?? I think an admin would have to do it.

About the notice thing, I have been using E_ALL and get zero notices.

As for the displayDebug() there is a specific reason for this. I included that so it is standalone. Later I will replace that with debug() and all my debug methods will use this debug function. The reason I pass the array like I do is so that when I extend the class I can modify the data before formating the array.

Why I can understand why you feel the way the exceptions are thrown is messy. I am undecided on what I will do. I will probably leave it how it is for now but when I refactor I will probably change it.

The reason there is no getData method is because the class was designed to be abstract. That is why a lot of the methods are protected. Because it is designed to be abstract I wanted a way to filter the debug data in each extended version, hence debug() passing data to _debug()

I will probably add that feature just for "completeness" but I don't think I would ever use it.

If you have any thoughts about what I said let me know. just remember the debug thing is kind of irrelevant because in my framework instead of debugDisplay() I would use debug() and debug would format an array into an html table via "new htmlTable()";

I am also thinking of making the template an iterator so i can iterate through the data.

The reason I am grouping stuff under $_option is because I was thinking more things will be added over time and it would be a better design decision to plan for it.

A lot of the things i ahve done to do with trying to make it abstract are because of how I have been planning to use it. But I will make it possible to have a few more public methods and remove the abstraction to some degree, but the class was designed to be abstract and that is how i intend on using it.

After I have removed the abstraction to some degree I think i will add php short tag support for servers that dont support short tags.

EDIT::
I just realized I forgot to say that I add comments after I am done, I just find it easier to code that way as the comments take up a lot of space and its quicker to code with out them. I have already added the comments i posted this post as soon as I was done with the class pretty much.

Posted: Mon Feb 05, 2007 8:24 pm
by Ollie Saunders
can I change it?? I think an admin would have to do it.
Click edit on your original post and change the subject field.
About the notice thing, I have been using E_ALL and get zero notices.
In PHP 5 E_ALL has the value of 2047 whilst E_STRICT has 2048 -> E_STRICT is not included as part of E_ALL. This will change in PHP 6.
As for the displayDebug() there is a specific reason for this. I included that so it is standalone. Later I will replace that with debug() and all my debug methods will use this debug function. The reason I pass the array like I do is so that when I extend the class I can modify the data before formating the array.
I don't believe standaloneness is a criteria I would choice to judge the quality of any design. Why it is necessary that this class can debug itself? If it is necessary that all the code exists in a single file then I would rather break the one class per file convention and have two classes than bloat one class with extra responsibilities.

The way it is now you can't extend debugging and templating behaviour separately.
The reason there is no getData method is because the class was designed to be abstract. That is why a lot of the methods are protected.
Why did you design it to be abstract? I see that decision as one that limits you.
Because it is designed to be abstract I wanted a way to filter the debug data in each extended version, hence debug() passing data to _debug()
OK that makes _debug() what is known as a "hook". Typically a comment is used when this is your intention.
I am also thinking of making the template an iterator so i can iterate through the data.
You could save yourself the effort and allow the data to be retrieved via a method - that could then be iterated over and a lot more. Just a suggestion
The reason I am grouping stuff under $_option is because I was thinking more things will be added over time and it would be a better design decision to plan for it.
If you can envisage that happening perhaps you should be breaking your templating down into smaller more specialist classes. It seems to me you are planning on having lots of hard coded logic and grouping flags for it all under an associative array and, again, you won't be able to extend different behaviours separately.
But I will make it possible to have a few more public methods and remove the abstraction to some degree, but the class was designed to be abstract and that is how i intend on using it.
Try not to attach yourself too firmly to ideas that you had at an early stage. You have become a bit fixated, presumably because you were pleased that you found a solution to a problem, but hey a better one might have come along, or not. The inventor of the Jet engine was shunned by some because it didn't have a propeller. If an inheritance based design is the best and you can justify it as so then fine otherwise you have learnt something.
I just realized I forgot to say that I add comments after I am done, I just find it easier to code that way as the comments take up a lot of space and its quicker to code with out them. I have already added the comments i posted this post as soon as I was done with the class pretty much.
Noted. It's just an obvious thing to mention when criticising.