0
\$\begingroup\$

I have created a PHP class to print into html easily from an array or object.

Using this class, you can get rid of multiple echo blocks or ugly concatenation while working with data arrays to transform into html.

class PrintEasy {
    /**
     * Print single data item with html
     * Include variable names within a [] block in the html string. 
     *
     * @param string $html
     * @param mixed $data Accepts both array and object format
     * @return boolean
     */
    public static function single($html, $data) {
        $newHtml = preg_replace_callback(
            '/\[([^\]]+)\]/',
            function($match) use ($data) {
                $key = $match[1];
                if(is_object($data)) {
                    return $data->$key;
                } elseif(is_array($data)) {
                    return $data[$key];
                } else return $match[0];

            },
            $html
        );
        echo $newHtml;
        return true;
    }

    /**
     * Print multiple items from a data array with html
     *
     * @param string $html
     * @param array $dataArray the dataArray can contain both array and object format data items
     * @param int $count (optional) If provided, only that number of items will be printed from the array
     * @return boolean
     */
    public static function multiple($html, $dataArray, $count = 0) {
        if(!$count) {
            $count = count($dataArray);
        }

        for($i=0;$i<$count;$i++) {
            PrintEasy::single($html, isset($dataArray[$i]) ? $dataArray[$i] : array());
        }
        return true;
    }
}

I have also created a github repo.

Are there any improvements I can make?

\$\endgroup\$

1 Answer 1

3
\$\begingroup\$

Readability issues

After staring at your code for about 5 minutes, I wasn't able to figure out what it was doing. So I copied it over to a PHP file and started testing it out. After some time I found out that it is replacing [name] in the HTML with the name key in an array provided as the $data parameter.

Your function names single() and multiple() are very non-descriptive. If I saw a piece of code doing PrintEasy::single($html, $data); I'd have no clue what is was doing other than printing some stuff. You want a line of code like that to be as descriptive as possible. This starts at naming your class and functions better.

On another note, I do not find the $count parameter on your second function very useful. Why wouldn't I just pass in a smaller array? The parameter most likely will only cause confusion. Furthermore, why not use the splat operator from PHP 5.6 to combine both of these methods into a single method with the same flexible functionality? This would result in a function like this:

function insert($html_string, ...$data) { // code

The name of your class is somewhat descriptive, as it does state that it will be printing something. However, we will be removing the echo with a return in a bit, so the class itself will no longer print something. So we need to come up with a better name for it. In this short period of time (good names can take time) I came up with VariableInserter, TemplateVariableInserter and PlaceholderReplacer.

I think I will be going with PlaceholderReplacer, as that is the only name that clearly indicates that the original text contains some placeholder strings that are going to be replaced.

Functionality

Your code does seem to work pretty well though. One tiny error that I noticed is that when your HTML contains for example [name], but there is no data object/array that contains this key, an undefined index/property NOTICE is raised.

Also, your PrintEasy class already decides for me what I want to do with the result. What if I want to use var_dump() instead, of if I want to use this functionality for an entirely different purpose? That is currently not possible with your implementation. Why not create a class that simply returns the replaced HTML. This makes it way more flexible. And if you do actually want to echo it, you just place echo in front of the function call.

Something else that struck me while looking at your code is that the implementation of your class is way more general than just for HTML. It can replace variables in any string. So why name it in such a specific way?

Rework of the class

Now that we have some better names for our class and functions, we can start working on improving the current code.

class PlaceholderReplacer {
    /**
     * Replaces placeholder strings such as "hello [name]" with
     * the 'name' key/property from an array/object
     * 
     * @param string $string
     * @param array|object ...$data_sources
     * @return string
     */
    public static function replaceWithData($string, ...$data_sources)
    {
        $new_string = preg_replace_callback(
            '/\[([^\]]+)\]/', 
            function ($match) use ($data_sources)
            {
                return self::retrieveMatchValueFromDataSources($match, $data_sources) ?: $match[0];
            },
            $string
        );

        return $new_string;
    }

    /**
     * @param array $match
     * @param array $data_sources
     * @return mixed|null
     */
    private static function retrieveMatchValueFromDataSources(array $match, array $data_sources)
    {
        $key = $match[1];

        foreach ($data_sources as $data_source)
        {
            if (is_object($data_source) && isset($data_source->$key))
            {
                return $data_source->$key;
            }

            if (is_array($data_source) && isset($data_source[$key]))
            {
                return $data_source[$key];
            }
        }

        return NULL;
    }
}

As you can see, I have extracted another private method from your original method. This method is responsible for trying to retrieve a key from any of the given data sources. By splitting up this functionality into a separate function your code becomes more readable and maintainable.

I have also added isset() checks before attempting to retrieve data from the data sources to prevent a NOTICE from being raised.

Readability of class usage

Now compare the use of your class to that of the class above. Assume these variables exist:

$html = "<body><h1> Hello [name]!</h1>";
$data_source = ["name" => "Chuck Norris"];

Now consider

PrintEasy::single($html, $data_source);

versus

echo PlaceholderReplacer::replaceWithData($html, $data_source);

Now you might argue that the functionality of your PrintEasy::single() method could be derived from looking at the $html and $data_source variables and I would somewhat agree. But keep in mind that often you don't directly see the contents of a string like this (think of a piece of text retrieved from a database). This would make it much harder to figure out what PrintEasy::single() is actually doing.

Reusability

Furthermore, without echoing the new string directly in the function but returning it, the functionality has become much more versatile. For example, I could rebuild your class using the new PlaceholderReplacer class:

class PrintEasy {
    function single($html, $data)
    {
        echo PlaceholderReplacer($html, $data);
    }

This example shows that there was some functionality in your class that could be extraced to a class of its own, with a single responsibility (SRP). Now all that is left of the printEasy class is to echo the result of another class. Now that just seems silly, so a dedicated PrintEasy class is no longer needed since we can just use echo outside of the class.

Testing

Another great aspect of returning instead of echoing is that it makes testing your code much easier.

Consider the following test:

class PlaceholderReplacerTest {
    testSimpleExample() {
        $html = "<body><h1> Hello [name]!</h1>";
        $data_source = ["name" => "Chuck Norris"];

        $new_html = PlaceholderReplacer::replaceWithData($html, $data_source);
        $this->assertEquals("<body><h1> Hello Chuch Norris!</h1>", $new_html);
    }
}

Your class did not return anything, but echos it directly, making it harder to test. (Not impossible however, since you could use ob_start() and ob_get_clean() to buffer the output and get the return value, but that is generally not something you want to be working with when testing other functionality).

\$\endgroup\$

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service and acknowledge you have read our privacy policy.

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