2
\$\begingroup\$

Is it better to validate and put an array value into a variable at the top of a file or to check the array value directly right before output in the file?

I work at a WordPress shop and we have a function that some devs commonly use to check the value of indexes in an array of custom field values:

//Check if an array has a key and return its value if so
function pkav($arr,$key){
    return isset($arr[$key]) ? $arr[$key] : false;
}

This leads to the following code in a template:

<?php
    $options = get_fields('options');
    $footer_text = pkav($options, 'footer_text');
    $footer_button = pkav($options, 'footer_button');
?>
<!-- Other code here -->
<?php if ( $footer_text || $footer_button ) : ?>
    <div class="col-12 col-lg-6">
        <?php if( $footer_text ) : ?>
            <h4><?php echo esc_html( $footer_text ); ?></h4>
        <?php endif; ?>
        <?php if( $footer_button ) : ?>
            <?php pk_output_button( $footer_button ); ?>
        <?php endif; ?>
    </div>
<?php endif; ?>

Some devs think this is more readable and a better way to code and check values/variables. It also allows us to neatly declare all variables at the top of a file so we immediately know what variables are used in a given template/template part. We have other devs who think pkav is unnecessary and an incorrect way to use variables. They'd rather see code like this:

<?php
    $options = get_fields('options');
?>
<!-- Other code here -->
<?php if ( ! empty( $options['footer_text'] ) || ! empty( $options['footer_button'] ) ) : ?>
    <div class="col-12 col-lg-6">
        <?php if( ! empty( $options['footer_text'] ) ) : ?>
            <h4><?php echo esc_html( $options['footer_text'] ); ?></h4>
        <?php endif; ?>
        <?php if( ! empty( $options['footer_button'] ) ) : ?>
            <?php pk_output_button( $options['footer_button'] ); ?>
        <?php endif; ?>
    </div>
<?php endif; ?>

Is there a right answer on what should be used in terms of php/wordpress/programming best practices/standards? The pkav functions seems to create more readable code by eliminating duplicate empty() checks and shortening each line but using only empty() checks seems to prevent creating extra variables.

\$\endgroup\$
  • \$\begingroup\$ Either way, pkav is a terrible function name. Or abbreviated: ew, pkav is a trbl fnn. See how ridiculously difficult it Is for a reader to understand abbreviated text? Its the same with source codes... \$\endgroup\$ – slepic Aug 26 at 3:34
  • \$\begingroup\$ @slepic I completely agree, I'm just trying to get some direction on which approach to take before I worry about that. \$\endgroup\$ – thenomadicmann Aug 26 at 3:57
  • \$\begingroup\$ Is this from your job? Are there other templates already written? If you have to choose from the two, choose whichever approach is already used in the other templates. Staying consistent throughout entire codebase is often the best approach. \$\endgroup\$ – slepic Aug 26 at 4:16
  • 1
    \$\begingroup\$ The title of your question must uniquely describe what your script does, not what your concerns are. \$\endgroup\$ – mickmackusa Aug 29 at 8:45
  • 1
    \$\begingroup\$ The current question title of your question is too generic to be helpful. Please edit to the site standard, which is for the title to simply state the task accomplished by the code. Please see How do I ask a good question?. \$\endgroup\$ – BCdotWEB Sep 25 at 13:50
0
\$\begingroup\$

I would not use the pkav function. Beside the fact that the name means nothing (which was stated in the comments) it also restricts the default value to false.
This is wrong when you might expect strings or something else for a certain variable.
From the looks of it, the footer text and button vars look like strings.

I would go with this

<?php
    $options = get_fields('options');
    $footer_text = $options['footer_text'] ?? '';
    $footer_button = $options['footer_button'] ?? '';
?>

I would also go with declaring all variables at the top of the file. It makes the code more readable and will allow a frontend developer to focus on the html part instead of trying to navigate in the if statements.

<?php if ($something) :?>
    <div><?= esc_html($something) ;?></div>
<?php endif;?>

is easier to read than

<?php if (!empty( $options['footer_text'])) : ?>
    <h4><?php echo esc_html($options['footer_text']); ?></h4>
<?php endif; ?>
| improve this answer | |
\$\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.