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>';
 }