Is this a good way to print output?

Question

I've learned PHP following only tutorials on the web and I'm quite sure my style is not at top.

I'd like to understand how this code could be written in a more elegant way:

<?
$posts = // array with informations about posts fetched with PHP PDO from DB

$html = "<ul>";
foreach($posts as $post) {
    $html .= "<li><h2>".$post["title"]."</h2><span>".$post["text"]."</span></li>";
}
$html .= "</ul>";
echo $html;

I'm not sure if this is already a good piece of code or not. I hope you guys can help me out.

Answer 1

There is nothing overtly incorrect with your code.

However, one thing that makes PHP nice to work with for web development is it's ability to interpolate variables. When you use double quotes around a string, PHP will interpolate (replace variables with their value). When you do not want that behavior, then you should use single quotes. You should also use single quotes to delimit keys in arrays, rather than double quotes, which will also interpolate variables, should they be found.

Here's what I'd do in your case, inside the foreach.

$html .= "<li><h2>{$post['title']}</h2><span>{$post['text']}</span></li>";

This looks more like html, and doesn't require dropping in and out of the string output or concatenating all over the place.

You might wonder why you need the blocks around the $post array variables. This is so that you can include the single quotes around the array key without confusing the php parser. If you were to try this:

$html .= "<li><h2>$post['title']</h2><span>$post['text']</span></li>";

You'll find you get a parse error. The block tells php to parse that code and insure it works fine. You can actually omit the single quotes and the code will parse, but it's better stylistically to always include them when dealing with array keys. This works without the block.

$html .= "<li><h2>$post[title]</h2><span>$post[text]</span></li>";

Whilst this might be a micro-optimization, when you omit the single quotes, php has to determine whether or not 'title' or 'text' are actually constants you've defined. It's also just in general, good practice to always reference variables in the same manner, for consistency and readability.

If you want your html markup to look a bit cleaner, it's also easy enough to add newlines using "\n".

$html = "<ul>\n";
foreach($posts as $post) {
    $html .= "<li><h2>{$post['title']}</h2><span>{$post['text']}</span></li>\n";

}
$html .= "</ul>\n";

One final note, since this is a code review -- what this code does, is intermix markup and data in a way that they can not be separated. There is a school of thought in the design pattern world, that data (models) should be pure, and that "views" should be responsible for output. For example, let's say that you want to provide this same data in json format for an ajax call you're doing someplace else in your code. This routine is not going to work, because you baked in the html markup.

One of the hallmarks of a spaghetti/php application is rampant intermixing of logic and HTML code, and views are a great way to avoid that from the get-go.

Your code will work, but it inherently has the weakness of intermixing your logic and data with your markup.

Template systems like smarty or twig to name a few (although most all of the PHP frameworks that provide MVC have a template system) encourage you to put your presentation (view) code into separate files. When it comes time to output your code, you inject the data into the view, and your view code can then create your output. You can do this in a lot of different ways, including using pure old PHP scripts, and alternative PHP syntax. I'd encourage you to look at some examples in the manuals of these template/view libraries.

As a simple example, here's how you could have your code in a php "view" file that you could require or include.

/* You might name this file postView.php */
/* You'll start in HTML mode */
<ul>
<? foreach($posts as $post): ?>
    <li><h2><?=$post['title']?></h2><span><?=$post['text']?></span></li>
<? endforeach; ?>
</ul>

Now your logic looks like:

<?php
// Fill your $post from the $_POST super glob.
// Do whatever other logic you need

// Time for output
require_once(postView.php);

Answer 2

Your code, while not wrong in a strict sense, is not as efficient as it can be. Think about what you're doing here:

$html = '<ul>';

This assigns a string constant to a variable $html. All well and good, but then, in your loop looks like this:

foreach($posts as $post)
{
    $html .= "<li><h2>".$post['title']."</h2><span>".$post['text']."</span></li>";
}
$html .= "</ul>";
echo $html;

Now what you should understand is that true concatenation isn't as easy as the syntax might have you believe it is. You're actually having to create a new string, and reassign the $html variable on each iteration of your string. You could think about your code as being a shorter version of writing:

$html = "<ul>"."<li><h2>".$posts[0]['title']."</h2><span>".$posts[0]['test']."</span></li>"."<li><h2>".$posts[1]['title']."</h2><span>".$posts[1]['text']."</span></li>"."</ul>";

Now this is, I think you'll agree, packed with pointless concatenation. Especially since you're then just passing the string to the echo language construct... Why not drop the variable all together?

echo '<ul>';
foreach($posts as $post)
{
    echo '<li><h2>',$post['title'], '</h2><span>', $post['text'], '</span></li>';
}
echo '</ul>';

And yes, those are comma's, not dots. By using comma's the values (string constants and variables) are passed to the echo language construct left to right. No strings are concatenated. Just like, if you've looked at the hello world expamples of C++ std::COUT << "Hello " << "world"; does.

Of course, echoing markup depends on the context in which it is used, but in a template/viewscript, one would more commonly write:

<ul>
<?php foreach ($posts as $post) { ?>
    <li><h2><?= $post['title'] ?></h2><span><?=$post['text'] ?></span></li>
<?php } ?>
</ul>

where <?= is equivalent to <?php echo. As opposed to the PHP short tag (<?) is always available, and doesn't require an ini-setting change.

Answer 3

A small extra suggestion: be aware that the above method will not send any content to the client until after you have constructed that (potentially huge string) in memory. It is better to directly echo the contents in this case. This will start sending your result HTML right away, which could help performance. This is especially the case if it takes a while to construct a full post, e.g.

<?
$posts = // array with minimal info

$html = "<ul>";
foreach($posts as $post) {
    if($post["display"]) {
        $fullPost = $postDao->fetch($post); // takes 30ms to fetch full post
        $html .= "<li><h2>".$fullPost["title"]."</h2><span>".$fullPost["text"]."</span></li>";
    }
}
$html .= "</ul>";
echo $html;