PHP color and image loop

Question

I did create a function which loops through the colors of a product and outputs a div with the specific color. After that it finds the image for the available colors. But I think the code can be a lot smarter.

Can someone optimize my code?

<?php
$brand_terms = get_the_terms($post, 'pa_kleur');
$brand_string = ''; // Reset string
foreach ($brand_terms as $term) : ?> <div style="display: block; margin-bottom: 50px;"><?php

    if (($term->name) == 'Roze') {
        echo '<div style="width: 20px; height: 20px; background-color: pink;" class="roze-kleur"></div>';

        $variations = $product->get_available_variations();
        foreach ( $variations as $key => $value ) {
    ?>
        <?php if ($value['attributes']['attribute_pa_kleur'] == 'roze') { ?>
        <li>
            <span><?php echo $value['image']['url']; }?></span>
        </li></div>
    <?php
    }
    }
    if (($term->name) == 'Grijs') {
        echo '<div style="width: 20px; height: 20px; background-color: grey;" class="grijze-kleur"></div>';

        $variations = $product->get_available_variations();
        foreach ( $variations as $key => $value ) {
    ?>
        <?php if ($value['attributes']['attribute_pa_kleur'] == 'grijs') { ?>
        <li>
            <span><?php echo $value['image']['url']; }?></span>
        </li></div>
    <?php
    }
    }
endforeach;
?>

Thanks for your time!

Answer 1

• keeping separate actions on a different line and tabbing your code will go a long way with making your code readable.
• avoid declaring single-use variables
• I don't see $brand_string being used, so that declaration can be omitted.
• I do not prefer the endforeach syntax. Despite being declarative, I find it unnecessarily verbose. Good tabbing is always enough to help me track the nesting of control structures and functions.
• move all inline styling to an external stylesheet. Using meaningful class names to assign appropriate colors is most logical here.
• I expect all <div> tags are display: block; by default -- I doubt this style declaration is necessary.
• use a lookup array to swiftly translate $term->name values into class attribute values and eliminate the mostly redundant if blocks.
• don't declare $key if you never intend to use it.
• filter the variations array based on the lowercase value of $term->name and use a conditional break for best efficiency. I am confidently assuming that there will only be one match in your variations array based on the trailing </div>
• your list item should be nested inside of a list tag (e.g. <ul>)
• remove the <span> tags -- they are unnecessary markup. If you need to style the list items, style the <li> tags
• if you must have your data processing in the same script as your content outputting, then I recommend doing your processing first, then keeping your markup readable by referencing the generated variables

Code:

$colorClasses = [
    'Roze' => 'roze-kleur',
    'Grijs' => 'grijze-kleur',
];
foreach (get_the_terms($post, 'pa_kleur') as $term) {
    $lowerColor = lcfirst($term->name);
    $colorUrl = 'you_decide_the_url';
    foreach ($product->get_available_variations() as $row) {
        if ($row['attributes']['attribute_pa_kleur'] === $lowerColor) {
            $colorUrl = $value['image']['url'];
            break;
        }
    }

    echo '<div>
              <div class="' , ($colorClasses[$term->name] ?? 'default-kleur') , '"></div>
              <ul><li>' , $colorUrl , '</li></ul>
          </div>';
 }