Myzwer / foothillschurch

Bootcamp II is a wordpress theme (as well as an inside joke) designed to suit the needs of foothillschurch.com. It makes use of webpack, Babel, Sass, Tailwind, Browsersync, PostCSS, ESLint, Stylelint, Prettier and more. It is meant for that site, but if you can use it by all means go for it.
1 stars 1 forks source link

Variable reassignment and some logic before it's needed. #56

Closed rain2o closed 1 month ago

rain2o commented 1 month ago

There are a couple of notes about this section.

https://github.com/Myzwer/foothillschurch/blob/effc5bbf81497ae99f0561ab7849412fe9272bfd/components/headers/default/_button.php#L38-L50

First, it's not great to use a variable for a field value, and then reassign it while in a condition checking its value. Also, you can avoid the final else and just start with null. I don't remember if echoing null into HTML from PHP rules in it showing as null, like <a href="" null>... But maybe try an empty string to be safe. Maybe try something like this.

$new_tab = get_sub_field( 'new_tab' );
$attrs = '';
if ( $new_tab == "yes" ) {
    $attrs = 'target="_blank"';
} elseif ( $new_tab == "cc" ) {
    $attrs = "data-open-in-church-center-modal='true'";
} ?>
<a href="<?php echo $link ?>"<?php echo $attrs ?>>

This also keeps the same type for $attrs (a string), which will be important when you start working with strict types one day (either in a language that's strictly typed, or by adding strict types to PHP).


My 2nd suggestion would be to move these conditions after if ( $link ):. Right now, even if you don't render a link, you're still executing the above logic, even if it's unused. Push that logic down to only happen if it's used.

So, the final code might look more like this

$link = get_sub_field( 'button_link' ) ?? get_sub_field( 'button_link_file' );
// Hide button if link is returning null
if ( $link ):
    // Get tab status
    $new_tab = get_sub_field( 'new_tab' );
    $attrs = '';
    if ( $new_tab == "yes" ) {
        $attrs = 'target="_blank"';
    } elseif ( $new_tab == "cc" ) {
        $attrs = "data-open-in-church-center-modal='true'";
    } ?>

    <a href="<?php echo $link ?>"<?php echo $attrs ?>>
rain2o commented 1 month ago

I just saw a lot of the same type of code in components/headers/default/_image.php, so the same suggestions apply there as well