Replacing variables with actual data

Question

I've written a class for loading a template file and replacing the variables with actual data. Please review my code and suggest improvements.

<?php
/**
 * This class will helpful for variable replacement from a loaded file
 * 
 * @author V V R
 * @version 1.0
 */
class Parser {
 public $dir = "";
 public $text = "";
 /**
 * To get directory location
 * @param $dir
 */
 function __construct($dir = "home/template") {
 $this->dir = $dir;
 }
 /**
 * To load the specified file from directory
 * @param $file
 */
 function loadFile($file) {
 try {
 if (file_exists ( $this->dir . "/" . $file )) {
 $this->text = file_get_contents ( $this->dir . "/" . $file );
 } else {
 throw new Exception ( $this->dir . "/" . $file . " does not exist" );
 }
 } catch ( Exception $e ) {
 error_log ( $e->getMessage () );
 die ();
 }
 }
 /**
 * To get the text from a file
 * @return text
 */
 function getText() {
 return $this->text;
 }
 /**
 * To replace the variables
 * @param unknown_type $var
 * @param unknown_type $text
 */
 function replace($var, $text) {
 $this->text = str_replace ( "{\$$var}", $text, $this->text );
 }
}
?>

• laurent: Have you considered using an existing solution, such as Mustache PHP?

Answer 1

Note: I'm sorry if this is completely irrelevant but I'm not entirely sure what you are trying to accomplish. If I'm way off base please let me know.

When I create a template loader I generally use output buffering with php include. This allows you to run the page as a php file without displaying its content before you are ready. The advantage to "parsing" your php files this way is that you can still run loops and functions.

This is how your loadFile method would look.

<?php
function loadFile($file, array $variables = null) {
 try {
 if (file_exists ( $this->dir . "/" . $file )) {
 // start output buffering
 ob_start();
 // create variables from the key/value pairs in the array
 if( is_array($variables) ) {
 extract($variables);
 }
 include_once $this->dir . "/" . $file;
 // store the output
 $this->text = ob_get_clean();
 // $this->text = file_get_contents ( $this->dir . "/" . $file );
 } else {
 throw new Exception ( $this->dir . "/" . $file . " does not exist" );
 }
 } catch ( Exception $e ) {
 error_log ( $e->getMessage () );
 die ();
 }
}
?>

As I said before, I'm not sure what kind of templates you are trying to load, but hopefully this will help.

• mseancole: I don't think its off topic at all. He asked for suggestions for better code and you provided it. +1

Answer 2

Properties

Your class properties probably shouldn't be visible outside of the class once they are set unless you do so with a getter method, like you have for the $text property. This is to say, the $dir and $text properties should not be public but private to prevent outside manipulations.

Public or Private

Your methods are not defined as either public or private. By default they will all be public, but it is better practice to just define them as such manually. Doubtful, but what if the default changes to private in a later version? Or what if PHP deprecates calling methods in this way? Your class will break. Always code with the future in mind.

loadFile()

As Baylor Rae said, it is more conventional to use output buffering when loading templates. He has given a good example, so I won't linger on it. Instead I will tell you how I would change your current loadFile() method to reuse less code. This can be used on Baylor's example as well.

function loadFile($filename) {
 $file = $this->dir . '/' . $filename;
 try {
 if (file_exists ( $file )) {
 $this->text = file_get_contents ( $file );
 } else {
 throw new Exception ( "$file does not exist" );
 }
 } catch ( Exception $e ) {
 error_log ( $e->getMessage () );
 die ();
 }
}

You shouldn't use die() in your scripts unless it is for debugging purposes. It is not very elegant and will leave your customers scratching their heads wondering where the rest of their content went. Instead I would have your loadFile() method return TRUE on success and FALSE on failure and then handle each individually in the controller.

Strings

Last thing I wish to leave you with. I see that you are using double quotes for all of your strings. This is a minor thing, but it is telling PHP that it should look in that string for variables to parse. If the string does not require parsing it should only use single quotes. Admittedly, this doesn't make a noticeable impact on your code, but it is something to keep in mind :)

Good luck!