1
\$\begingroup\$

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);
      }
   }
}
\$\endgroup\$
2
\$\begingroup\$

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.

\$\endgroup\$
  • \$\begingroup\$ 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); \$\endgroup\$ – Moses Mar 4 '12 at 5:08
2
\$\begingroup\$

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);
}
\$\endgroup\$

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service, privacy policy and cookie policy

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