Tell me more ×
Code Review Stack Exchange is a question and answer site for peer programmer code reviews. It's 100% free, no registration required.
up vote 4 down vote favorite
1

I written a class for loading an template file and replacing the variables with actual data.

Please review my code and suggest me the best code.

My Code:

<?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 );
    }
}

?>
share|improve this question
Have you considered using an existing solution, such as Mustache PHP? – Laurent Jun 1 '12 at 7:19

2 Answers

up vote 6 down vote accepted

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.

share|improve this answer
2  
I don't think its off topic at all. He asked for suggestions for better code and you provided it. +1 – mseancole Jun 1 '12 at 13:36
Thanks for suggestion – vvr Jun 4 '12 at 10:46
up vote 3 down vote

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!

share|improve this answer
Thanks for suggestion – vvr Jun 4 '12 at 10:46

Your Answer

 
discard

By posting your answer, you agree to the privacy policy and terms of service.

Not the answer you're looking for? Browse other questions tagged or ask your own question.