Jump to content

Is there a better way to code this?


Stick
 Share

Recommended Posts

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>

 

  • Like 1
Link to comment
Share on other sites

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>
  • Like 3
Link to comment
Share on other sites

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>

 

  • Like 1
Link to comment
Share on other sites

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

  • Like 1
  • Thanks 1
Link to comment
Share on other sites

Create an account or sign in to comment

You need to be a member in order to leave a comment

Create an account

Sign up for a new account in our community. It's easy!

Register a new account

Sign in

Already have an account? Sign in here.

Sign In Now
 Share

  • Recently Browsing   0 members

    • No registered users viewing this page.
×
×
  • Create New...