1
\$\begingroup\$

I've got this code to work, but being really new to PHP, I don't know if this is proper. This is my first time using PHP to increment.

  1. I have 10 fields for YouTube videos (using Advanced Custom Fields in WordPress - I don't have the repeater fields and won't be getting them)
  2. The video/html elements only show if the corresponding video field is not empty

Are the variables correctly placed?

Are there better ways to do this?

    <?php
         $vidnum = 'video_';
         for($n=0; $n<=9; $n++) {
         $next = $vidnum.($n +1 );
         $video = get_field('' . $next . '');
             if (!empty($video))
                    echo '<div class="video ' . $next . '">' . $video . '</div>';
                }

        ?>
\$\endgroup\$
1
\$\begingroup\$
     $vidnum = 'video_';

Presumably $vidnum is short for $video_number, but this isn't a video number. It's the prefix before the video number.

     $video_number_prefix = 'video_';

You can go ahead and call it what it is.

     for($n=0; $n<=9; $n++) {
     $next = $vidnum.($n +1 );

You say that you have ten fields but you write 9. There are two ways to write it with 10.

     for ( $n = 0; $n < 10; $n++ ) {

But the second works better with your $next variable, which I'm going to rename to $video_identifier.

     for ( $i = 1; $i <= 10; $i++ ) {
         $video_identifier = 'video_' . $i;

You don't need to save the value of the string literal. You only use it once, so you can just use it.

Starting with 1 instead of 0 means that you don't have to add 1 to the value before using it.

I switched from $n to $i, as $i is a more common loop iteration variable.

     $video = get_field('' . $next . '');

This seems more complicated than it needs to be.

         $video = get_field($video_identifier);

Adding the empty strings doesn't seem to accomplish anything.

         if (!empty($video))
             echo '<div class="video ' . $next . '">' . $video . '</div>';

The single statement version of an if is harder to read and maintain.

         if ( ! empty($video) ) {
             echo '<div class="video ' . $video_identifier . '">' . $video . '</div>' . "\n";
         }

I also added a line break at the end. Functionally this won't matter, but it makes the HTML easier to read.

The whole thing:

<?php
     for ( $i = 1; $i <= 10; $i++ ) {
         $video_identifier = 'video_' . $i;
         $video = get_field($video_identifier);
         if ( ! empty($video) ) {
             echo '<div class="video ' . $video_identifier . '">' . $video . '</div>' . "\n";
         }
     }
?>

This also fixes your indentation issues.

Note that I'm not commenting on your use of get_field. Looking at the documentation, it seems that get_field depends heavily on the configuration. Without the configuration, that is off-topic for review.

\$\endgroup\$
  • \$\begingroup\$ whoa, great answer. Works just perfectly. get_field is just part of Advanced Custom Fields - so no worries about that. \$\endgroup\$ – alexwc_ Feb 4 '15 at 2:28

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.