Is there a better way to code this?

[Stick]

I am new to PHP and new to PW.  The below code works but I get the feeling it could be cleaner/better.  Any assistance would be greatly appreciated.

  <div class="row mb-2">
  <?php
    $items = $pages->find("homepage_item=1");
      foreach($items as $item) {
	  // $item === $pages->get($item->id);
	  echo "<div class='col-md-6'>";
	    echo "<div class='row g-0 border rounded overflow-hidden flex-md-row mb-4 shadow-sm h-md-250 position-relative'> <div class='col p-4 d-flex flex-column position-static'>";
	      echo "<h3 class='mb-0'>$item->title</h3>";
	      if($item->arbitrary_publish_date) { echo "<div class='mb-1 text-muted'>$item->arbitrary_publish_date</div>";
          }
          else 
          {echo "<div class='mb-1 text-muted'></div>";
          }
	      echo "<p class='card-text mb-auto'>$item->summary</p>";
	      echo "<a href='$item->url' class='stretched-link'>Continue reading</a>";
	    echo "</div><div class='col-auto d-none d-lg-block'>";
	      echo "<svg class='bd-placeholder-img' width='200' height='250' xmlns='http://www.w3.org/2000/svg' role='img' aria-label='Placeholder: Thumbnail' preserveAspectRatio='xMidYMid slice' focusable='false'><title>Placeholder</title><rect width='100%' height='100%' fill='#55595c'/><text x='50%' y='50%' fill='#eceeef' dy='.3em'>Check it out!</text></svg>";
	    echo "</div>";
	    echo "</div>";
	    echo "</div>";
	  // echo $item->title;
    }
    ?>
    </div>

 

[Kiwi Chris]

How about something like this?

This way your editor can recognise the HTML tags as well as the PHP tags. The <?= ?> tag is a PHP shorthand for echo ... that you can insert within HTML. 

For PHP logic you need the full <?php ... ?> tag. You'll notice I've left a couple of echo statements in because they're short, and it's not really worth closing the PHP tag and opening another one for such short sections of HTML code.

  <div class="row mb-2">
  <?php
    $items = $pages->find("homepage_item=1");
      foreach($items as $item) {
	  // $item === $pages->get($item->id);
      ?>
	  <div class='col-md-6'>
	  	<div class='row g-0 border rounded overflow-hidden flex-md-row mb-4 shadow-sm h-md-250 position-relative'> <div class='col p-4 d-flex flex-column position-static'>
	    	<h3 class='mb-0'><?= $item->title?></h3>
	      <?php if($item->arbitrary_publish_date) { 
        	echo "<div class='mb-1 text-muted'>$item->arbitrary_publish_date</div>";
          }
          else 
          {
          	echo "<div class='mb-1 text-muted'></div>";
          }
        ?>
	     <p class='card-text mb-auto'><?= $item->summary ?></p>
	     <a href='<?= $item->url ?>' class='stretched-link'>Continue reading</a>
	    </div>
        <div class='col-auto d-none d-lg-block'>
	    <svg class='bd-placeholder-img' width='200' height='250' xmlns='http://www.w3.org/2000/svg' role='img' aria-label='Placeholder: Thumbnail' preserveAspectRatio='xMidYMid slice' focusable='false'><title>Placeholder</title><rect width='100%' height='100%' fill='#55595c'/><text x='50%' y='50%' fill='#eceeef' dy='.3em'>Check it out!</text></svg>
	    </div>
	    </div>
	    </div>
   <?php
	  // echo $item->title;
    }
    ?>
    </div>

[Stick]

Awesome! Thank you, this is far easier for me to wrap my head around. ?

[taotoo]

I'm new to both too, but thought I'd chip in with the below as an alternative. May not be optimal, but it's how I've ended up doing things, coming from HTML.

	<?php $items = $pages->find("homepage_item=1"); ?>
	<div class="row mb-2">
		<?php foreach($items as $item): ?>
		<div class='col-md-6'>
			<div class='row g-0 border rounded overflow-hidden flex-md-row mb-4 shadow-sm h-md-250 position-relative'>
				<div class='col p-4 d-flex flex-column position-static'>";
					<h3 class='mb-0'>$item->title</h3>
					<?php if($item->arbitrary_publish_date): ?>
						<div class='mb-1 text-muted'><?php $item->arbitrary_publish_date; ?></div>
					<?php else: ?>
						<div class='mb-1 text-muted'></div>
					<?php endif; ?>
					<p class='card-text mb-auto'><?php $item->summary; ?></p>
					<a href='$item->url' class='stretched-link'>Continue reading</a>
				</div>
				<div class='col-auto d-none d-lg-block'>
					<svg class='bd-placeholder-img' width='200' height='250' xmlns='http://www.w3.org/2000/svg' role='img' aria-label='Placeholder: Thumbnail' preserveAspectRatio='xMidYMid slice' focusable='false'>
						<title>Placeholder</title>
						<rect width='100%' height='100%' fill='#55595c' />
						<text x='50%' y='50%' fill='#eceeef' dy='.3em'>Check it out!</text>
					</svg>
				</div>
			</div>
		</div>
		<?php endforeach; ?>
	</div>

 

[bernhard]

I also prefer to use foreach + endforeach or if + else + endif when working in markup. It's a lot cleaner to read than single brackets being somewhere in the file within <?php and ?>

You can get rid of the if/else though because you are outputting the wrapping div of $date anyhow. This is how I would format your code:

<?php
$items = $pages->find("homepage_item=1");
$date = $item->arbitrary_publish_date;
?>
<div class="row mb-2">
  <?php foreach($items as $item): ?>
  <div class='col-md-6'>
    <div class='row g-0 border rounded overflow-hidden flex-md-row
      mb-4 shadow-sm h-md-250 position-relative'>
      <div class='col p-4 d-flex flex-column position-static'>";
        <h3 class='mb-0'><?= $item->title ?></h3>
        <div class='mb-1 text-muted'><?= $date ?></div>
        <p class='card-text mb-auto'><?= $item->summary; ?></p>
        <a href='<?= $item->url ?>' class='stretched-link'>Continue reading</a>
      </div>
      <div class='col-auto d-none d-lg-block'>
        <svg class='bd-placeholder-img' width='200' height='250'
          xmlns='http://www.w3.org/2000/svg' role='img'
          aria-label='Placeholder: Thumbnail' preserveAspectRatio='xMidYMid slice'
          focusable='false'>
          <title>Placeholder</title>
          <rect width='100%' height='100%' fill='#55595c' />
          <text x='50%' y='50%' fill='#eceeef' dy='.3em'>Check it out!</text>
        </svg>
      </div>
    </div>
  </div>
  <?php endforeach; ?>
</div>

@taotoo you have some missing echo statements in your code

Note that I'm also wrapping code after 80chars usually, see https://stackoverflow.com/questions/578059/studies-on-optimal-code-width