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 1 down vote favorite

As you can see from the code below, my set function accepts two types of parameters: either strings or arrays. Now, ideally I would like to be using operator overloading to handle this, but since PHP doesn't offer anything like that I am stuck with code like this. Is there a better way to write this (or perhaps a better way to address the problem altogether)?

class View {
   // stores file passed to View
   private $template;

   // setter function for template variables
   public function set($var, $content) {
      if (is_array($var) && is_array($content)) {
         if (sizeof($var) == sizeof($content)) {
            foreach ($var as $vari) {
               foreach ($content as $contenti) {
                  $this->template = str_replace("{" . "$vari" . "}", $contenti, $this->template);
               }
            }
         }
      }
      else {
         $this->template = str_replace("{" . "$var" . "}", $content, $this->template);
      }
   }
}
share|improve this question

2 Answers

up vote 1 down vote accepted

First, there is a simple way to flatten out those loops, but it is not what I would suggest. Here is the simple way:

if ((sizeof($var) == sizeof($content))) {
    $count = sizeof($var);

    for ($i =0; $i < $count; $i++) {
        $this->template = str_replace(
            '{' . $var[$i] . '}', $content[$i], $this->template);
    }
}

Note that you are currently silently doing nothing when the size of your arrays don't match. I think that is a bug.

My Suggestions

$var and $content are not very descriptive: I would suggest $match and $replacement.

str_replace can handle arrays as input. I would suggest filling your match with the '{' and '}' beforehand. The code then becomes:

public function set($match, $replacement)
{
   $this->template = str_replace($match, $replacement, $this->template);
}

I would place any sanity checks before the str_replace that you require. str_replace uses blank replacements when the matches array is larger than the replacements. If you want to avoid that use:

   if (is_array($match) && is_array($replacement) &&
       sizeof($match) > sizeof($replacement))
   {
      throw new InvalidArgumentException(
         __METHOD__ . ' Each match must have a replacement');
   }

This avoids arrow code which will make the code easier to test, reduce its complexity and make it easier to maintain.

share|improve this answer
Interesting, I had no idea I could pass arrays directly into str_replace and it would handle the iteration for me behind the scenes! Additionally, I believe you forgot to include the third parameter in your str_replace function; shouldn't it read: $this->template = str_replace($match, $replacement, $this->template); – Moses Mar 4 '12 at 5:08
Thanks for that, I have edited it in now. – Paul Mar 4 '12 at 5:24
up vote 1 down vote

You could use preg_replace_callback to iterate through your array and replace any data. 

public function set($data){ // data = array

    $callback = function ($matches) use ($data)
    {
            return ( isset($data[$matches[1]]) ) 
               ? $data[$matches[1]] 
               : $matches[0];
    };

     return preg_replace_callback(
                '/\{(.*?)\}/', 
                $callback, 
                $this->template);
}
share|improve this answer

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.