3
\$\begingroup\$

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!

\$\endgroup\$
3
\$\begingroup\$
  • 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>';
 }
| 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.