1
\$\begingroup\$

I am writing up a class to handle various templates to be used in a web application. Each template contains various placeholders which will need to be replaced at the time of the build. I am wondering if the method I am using is the best solution, or if I should consider something else. At the moment I have two separate functions addPlaceholder() and addMultiplePlaceholders(). Each performs as their names suggest.

The function addPlaceholder() has two parameters one, the key $k, must be a string while the other, the value $v, can be anything. These get stored in the classes placeholder array.

The function addMultiplePlaceholders() has only one parameter, $array, and it should be an associative array. This function calls addPlaceholder() for each index of $array to store the corresponding values. The reason for the call to an existing function is to help alleviate duplicitous code, and when only a single placeholder is in use to bypass the foreach loop.

Is this the best method to accomplish this? Would it be better to forgo addPlaceholder() and just use addMultiplePlaceholders() for everything? Perhaps, overloading addPlaceholder() would be a better solution having one version which accepts the two parameters, and another which accepted the array. However, I would be in the same situation as now having to decide whether to duplicate the code for adding the placeholder to the class property, or calling the other function.

public function addPlaceholder($k,$v){
    if(empty($k)){
        throw new Exception('Class Template->addPlaceholder() '.'The placeholder string cannot be empty.');
    }

    if(isset($this->placeholders[$k])){
        throw new Exception('Class Template->addPlaceholder() '.'['.$k.'] is a duplicate entry');
    }

    $this->placeholders[$k] = $v;
}

public function addMultiplePlaceholders($array){
    if(!is_associative($array)){                    // if `$array` is not an associative array throw exception
        throw new Exception("The supplied parameter value is not an associative array");
    }

    foreach($array as $key => $value){
        try{
            addPlaceholder($key,$value);
        }catch(Exception $e){
            $msg = $e->getMessage();
            preg_replace(preg_quote('Class Template->'), '', $msg);
            throw new Exception("Class Template->addMultiplePlacerholders()->".$msg);
        }
    }
}
\$\endgroup\$
1
  • 2
    \$\begingroup\$ Your solution sounds fine. By the way, "duplicitous" means "treacherous" and has nothing to do with duplicates :) \$\endgroup\$ – Wayne Dec 23 '13 at 18:51
3
\$\begingroup\$

To answer just the question in the title; Your solution seems fine to me; addMultiplePlaceholders() is a utility method that acts as a wrapper for addPlaceholder()

Note there's a bug in your code; addMultiplePlaceholders() calls addPlaceholder() without $this-> 'prefix', so your code won't work

However, there is room for improvement. I'll try to summarize what I'd do differently.

Remove redundant information from your Exception messages

Exceptions are 'smart' messages in that they already contain all the meta-information you need to find where they occurred (e.g. line number, file etc). Don't clutter your messages with that information, It'll only complicate your code (for example the preg_replace). Just include the bare error-message. See the documentation on Exceptions.

If your going to output a message, make sure they cannot be mis-interpreted. Being consistent in your messages will prevent confusion and make it easier to debug your code later on, for example:

throw new Exception('The placeholder string cannot be empty');

Is confusing; What is the placeholder string? The name ($k)? Value ($v)?

Outputting the meta-information can be done when printing the exception, for example;

Just use this to throw your exception;

throw new Exception('Placeholder name cannot be empty');

And output it in your Exception-handler with this;

printf(
    "An Exception has occurred: '%s' in file '%s' on line '%s',
    $e->getMessage(),
    $e->getFile(),
    $e->getLine()
);

// --> An Exception has occurred: 'The placeholder string cannot be empty' in file 'Template.php' on line '52'

Since you're no longer including the name of the Class/Method in your messages, you can completely remove the 'try/catch' from your 'addMultiplePlaceholders'.

Use the right 'type' of Exception

To be more expressive/specific when throwing Exceptions, consider using special Exceptions for a situation, for example, the SPL Exceptions. In this case an InvalidArgumentException will be appropriate

Consider what really is an exception

Inside addPlaceholder, you're throwing an exception if a key is already set. Personally, I wouldn't do this (it does feel like 'business logic'). Technically, overwriting the value of a placeholder that was already set won't cause any problem. Disallowing overwriting existing placeholders may limit you in situations that you want to overwrite a placeholder.

However, renaming addPlaceholder to setPlaceholder may be more appropriate, to indicate that you can both 'add' or 'update/replace' a placeholder.

Use descriptive variable/argument-names

The arguments for addPlaceholder are not very descriptive. Additionally, the variable names you're using seem to be picked based on the the 'inner workings' of the method (key/value pairs), which should be of no interest when using that method. I'd suggest something more describing the purpose of the arguments, for example:

public function addPlaceholder($name, $value)
{
    //...
}

Likewise, replace '$array' to something more describing its purpose, for example

public function addMultiplePlaceholders($nameValuePairs)
{
    //...
}

// or..

public function addMultiplePlaceholders($placeholders)
{
    //...
}

If you want to explicitly indicate you're expecting an array (and have PHP check this for you), you can add a type-hint, like this:

public function addMultiplePlaceholders(array $placeholders)
{
    //...
}

Finally, naming this method addMultiplePlaceholders may be a bit verbose. Since the method is plural, it's probably already clear that it is used to add multiple placeholders, so simply calling it addPlaceholders may be clear enough.

Remove 'comment' clutter. Add PhpDoc in stead

Do not describe code that is self-explanatory. In general, try to omit inline comments in all cases, unless a piece of code really is confusing.

In most cases, don't describe the code, but describe the purpose / intent of a function/method inside a PhpDoc comment.

For example:

if(!is_associative($array)){ // if `$array` is not an associative array throw exception
    throw new Exception("The supplied parameter value is not an associative array");
}

This comment is just repeating the code! Also, inline code is not used by IDE's and will not be included in automatically generated documentation (e.g. phpdocumentor). It's best to move relevant information to a PhpDoc block

For example:

/**
 * Adds placeholder(s) passed as an associative array ('name' => value)
 *
 * Name should be a string, value can be any type of value
 *
 * This method is a wrapper for @see addPlaceholder()
 *
 * @param array $nameValuePairs
 *
 * @throws InvalidArgumentException if $nameValuePairs is not an associative array 
 *                                  or an empty name is used for a placeholder
 * @return void
 */
public function addMultiplePlaceholders(array $nameValuePairs)
{
    //...
}

Modified code

This is the code after applying my suggestions

/**
 * Adds or updates a placeholder
 *
 * @param string $name  Name of the placeholder. Cannot be empty
 * @param mixed  $value Value of the placeholder
 *
 * @throws InvalidArgumentException if $name is invalid (empty)
 *
 * @return void
 */
public function setPlaceholder($name, $value)
{
    if (empty($name)) {
        throw new InvalidArgumentException('Placeholder $name cannot be empty.');
    }

    $this->placeholders[$name] = $value;
}

/**
 * Adds or updates placeholder(s), passed as an associative array ('name' => value)
 *
 * Name should be a string, value can be any type of value
 *
 * This method is a wrapper for @see addPlaceholder()
 *
 * @param array $placeholders placeholder(s), passed as an associative array ('name' => value)
 *
 * @throws InvalidArgumentException if $placeholders is not an associative array 
 *                                  or an empty name is used for a placeholder
 * @return void
 */
public function setPlaceholders(array $placeholders)
{
    if (!is_associative($nameValuePairs)) { 
        throw new InvalidArgumentException('Parameter $placeholders is not an associative array');
    }

    foreach (placeholders as $name => $value) {
        $this->addPlaceholder($name, $value);
    }
}
\$\endgroup\$
2
  • \$\begingroup\$ Wow, and thank you!! This response is far more than I had expected. I have been doing some research on the how to properly use exceptions this is my first attempt. Again, thank you. \$\endgroup\$ – Brook Julias Dec 28 '13 at 5:25
  • \$\begingroup\$ @BrookJulias you are welcome. My description of Exception handling is nowhere near complete, but it might give you some ideas. Be sure to check out the source code of some popular frameworks (e.g. Symphony, CakePHP). It's a good source to obtain ideas and learn. \$\endgroup\$ – thaJeztah Dec 28 '13 at 16:33
0
\$\begingroup\$

Two remarks:

I share your "feeling" that this is somehow a "code duplication": Writing

addPlaceHolder(array("key" => someValue))

is equally understandable and not that much more effort to write than

addPlaceHolder("key", someValue")

Also providing only the version with multiple key/value pairs will encourage "clients" to pass values "in a single step" which I consinder to be a good thing (otherwise, filling the template might be unnecessarily cluttered and thus seeing which values go into the template might be more difficult).

Therefore I'd probably go with only one function.

Also the addMultiplePlaceholders function can be easily implemented using PHP's array_merge builtin. Detecting duplicate keys can be done using array_intersect_keys, and you can also simply check for the "empty" key in the argument array. So the function could look like

public function addPlaceholders($placeholders){
    if(!is_associative($array)){ 
        throw new Exception("The supplied parameter value is not an associative array");
    }
    if (array_key_exists("", $placeholders)) {
        throw new Exception('The placeholder string cannot be empty.');
    }
    $intersect = array_intersect_key($placeholders, $this->placeholders);
    if (count($intersect) > 0) {
        throw new Exception('Duplicate entries: ' . implode(', ', array_keys($intersect)));
    }
    $this->placeholders = array_merge($this->placeholders, $placeholders);
}
\$\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.