# Array that defines product information and generates the html for the product

Is there a more productive way to do this? Is my code up to standards or up to best practices?

   <?php

/*
FILE:       index.php
AUTHOR:     Darcey Mckelvey
PURPOSE:    View Products
*/

// Define product information
$products[0] = array( 'name' => 'Sneakers', 'price' => 24.99, 'category' => 'Shoes', 'description' => 'Black sneakers. Good for walking or athletic activity' );$products[1] = array(
'name' => 'Armani Red Silk Shirt',
'price' => 379.99,
'category' => 'Shirts',
'description' => 'Fancy Italian red silk shirt. Very spiffy, sure to attract the ladies'
);

echo "<h2 style='text-align:center;'>Welcome to DropShop!</h2>
<h3>Our Products</h3>";

foreach($products as$product) {
echo "<p><span style='font-weight:bold;'>" . $product['name'] . "</span><br />" . "Price:$" . $product['price'] . "<br />" . "Category: " .$product['category'] . "<br />" .
$product['description'] . "</p>"; } ?>  ## 2 Answers Well, if this is the real code and not a sample (like, data you're getting in $products is not fetched from the database), then there isn't much use of hard-coding a PHP array just to immediately use it to display HTML paragraphs - why not just write everything as HTML?

Other than that, you're implicitly creating an array (there's no $products = array() first). It's not an error, but it can be a little confusing. My immediate reaction was "where is $product coming from?". Also, you don't need to declare each element separately, you can do everything "at once" like this:

$products = array( array( 'name' => 'Sneakers', 'price' => 24.99, 'category' => 'Shoes', 'description' => 'Black sneakers. Good for walking or athletic activity' ), array( 'name' => 'Armani Red Silk Shirt', 'price' => 379.99, 'category' => 'Shirts', 'description' => 'Fancy Italian red silk shirt. Very spiffy, sure to attract the ladies' ) );  Also, mixing PHP and HTML in single file isn't good practice, except when you're just echo-ing data in a "view". Other than that, logic should be separated. There is not very much going on in your code. It is basically just a template, so not very much room for improvement when it comes to the logic. It could be a bit better when it comes to style and readability though. Allow me to refactor the code to the way I would write it: <?php$products = [
[
'name'        => 'Sneakers',
'price'       => 24.99,
'category'    => 'Shoes',
'description' => 'Black sneakers. Good for walking or athletic activity',
],
[
'name'        => 'Armani Red Silk Shirt',
'price'       => 379.99,
'category'    => 'Shirts',
'description' => 'Fancy Italian red silk shirt. Very spiffy, sure to attract the ladies',
],
];
?>
<h2 style='text-align:center;'>Welcome to DropShop!</h2>
<h3>Our Products</h3>

<?php foreach ($products as$product) : ?>
<p>
<span style='font-weight:bold;'>
<?= $product['name'] ?> </span> <br/> Price:$<?= $product['price'] ?> <br/> Category: <?=$product['category'] ?>
<br/>
<?= $product['description'] ?> </p> <?php endforeach; ?>  What I did: • I declare all the products at once, and use the short [] array notation. I find it a lot cleaner (and it's less typing) • Most of the code is in fact html, so in stead of staying in php mode, I closed the php and started writing ordinary html in stead of echoing a concatenated string. Imo this is a lot less prone to errors. You are bound to miss a . or use the wrong type of quote somewhere when you concatenate all those strings. Also it is a lot easier to read, and your IDE should be able to spot errors in your html a lot easier this way. • Note that I use the short echo notation <?= 'hello' ?> in stead of the more verbose <?php echo 'hello' ?>. It is perfectly safe to use since php 5.4 and I find it cleaner. • On the other hand I do use the more verbose foreach() : ... endforeach;. I find it tricky sometimes to keep track of the braces inside templates and this adds to the readability imo. Allow me to comment on @OliverMaksimovic remark about going for straight html in stead of using an array. I do believe it is much better to use an array here. It makes the code a lot easier to maintain (what if you want to change the markup of a product, you would have to make that change multiple times). Also, if you ever decide to fetch those products from a database or something, you could basically keep your template as is, you would just have to change the$products declaration.

I could go on about how I would use Objects for those products in stead of arrays, and there remain a few things to be said about the markup you are using (it is not very semantic, it should probably be a ul of articles at the least), but I think I gave you enough food for thought now.

I hope it helps, and do feel free to ask if anything is unclear!