0
\$\begingroup\$

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>";


    // Loop to display all products
    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>";
    }

    ?>
\$\endgroup\$
2
\$\begingroup\$

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.

\$\endgroup\$
0
\$\begingroup\$

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!

\$\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.