Displaying images based on column colors

Question

This code displays a lot of images based on color in columns. I'm thinking it can probably be optimized a lot better. But I'm wondering if anyone has a more fundamental solution on how this could be improved in regards to speed and legibility.

The database has the equivalent of this XML file.

Any suggestions?

CSS

#row {
 margin-top: 0px;
 margin-bottom: 0px;
 padding: 0;
 display: table-row;
}
#col0, #col1, #col2, #col3, #col4, #col5 {
 margin-top: 0px;
 margin-bottom: 0px;
 padding: 0;
 display: table-cell;
}

PHP

<?php 
 $url_core_sets = "cards/HQ/Core Sets/";
 $url_expansion = "cards/HQ/Expansions/";
 $url_promoCards = "cards/HQ/Promo Cards/";
 $url_special_sets = "cards/HQ/Special Sets/";
 $url_low_rez = "cards/HQ/Low Rez/"; // low resolution cards (cards that are used when no alternative is available)
 $filetype = ".jpg";
 $width = 240;
 $height = 340;
 // figure out which folder the cards are in...
 if (file_exists($url_core_sets.$cards[0]['set'])) {
 $url = $url_core_sets;
 } else if (file_exists($url_expansion.$cards[0]['set'])) {
 $url = $url_expansion;
 } else if (file_exists($url_promoCards.$cards[0]['set'])) {
 $url = $url_promoCards;
 } else if (file_exists($url_special_sets.$cards[0]['set'])) {
 $url = $url_special_sets;
 } else if (file_exists($url_low_rez.$cards[0]['set'])) {
 $url = $url_low_rez;
 }
 // figure out file type...
 if(file_exists($url."/".$cards[0]['name'].".full.jpg")) {
 $filetype = ".full.jpg";
 }
 else {
 $filetype = ".jpg";
 }
 class CardData {
 public $name;
 public $full_path;
 public $cost;
 }
 $colors = array("B", "R", "G", "W", "U");
 $color_index = 0;
 $current_color = $colors[$color_index];
 $columns = array(array(), array(), array(), array(), array(), array());
 for($i=0; $i<count($cards); $i+=1) {
 $card_data = new CardData();
 $card_data->name = htmlentities($cards[$i]['name']);
 $card_data->full_path = htmlentities($url.$cards[$i]['set']."/".$cards[$i]['name'].$filetype);
 $card_data->cost = $cards[$i]['cost'];
 $pos = strpos($card_data->cost, $current_color);
 for($j=0; $j<count($colors); $j+=1) { // check if other colors are being used in the current card
 if(strcmp($colors[$j], $current_color) != 0) { // if it's not the current color
 $other_colors = strpos($card_data->cost, $colors[$j]);
 if($other_colors !== false) { // different color is being used
 break;
 }
 }
 }
 if ($pos === false || $other_colors !== false) { // color doesn't exists in current card OR different color is being used
 if($color_index < 5) {
 $color_index += 1; // next column
 $current_color = $colors[$color_index]; // keep track of current color
 }
 }
 array_push($columns[$color_index], $card_data);
 }
 $col0Index = 0;
 $col1Index = 0;
 $col2Index = 0;
 $col3Index = 0;
 $col4Index = 0;
 $col5Index = 0;
 while($col0Index != -1 || $col1Index != -1 || $col2Index != -1 || $col3Index != -1 || $col4Index != -1 || $col5Index != -1) {
?> 
 <div id="row">
 <div id="col0">
 <?php
 if($col0Index != -1) {
 $name = $columns[0][$col0Index]->name;
 if($name != "") {
 $full_path = $columns[0][$col0Index]->full_path;
 echo '<img src="'.$full_path.'" alt="'.$name.'" width="'.$width.'" height="'.$height.'">';
 $col0Index+=1;
 }
 else {
 $col0Index = -1;
 }
 }
 ?>
 </div>
 <div id="col1">
 <?php
 if($col1Index != -1) {
 $name = $columns[1][$col1Index]->name;
 if($name != "") {
 $full_path = $columns[1][$col1Index]->full_path;
 echo '<img src="'.$full_path.'" alt="'.$name.'" width="'.$width.'" height="'.$height.'">';
 $col1Index+=1;
 }
 else {
 $col1Index = -1;
 }
 }
 ?>
 </div>
 <div id="col2">
 <?php
 if($col2Index != -1) {
 $name = $columns[2][$col2Index]->name;
 if($name != "") {
 $full_path = $columns[2][$col2Index]->full_path;
 echo '<img src="'.$full_path.'" alt="'.$name.'" width="'.$width.'" height="'.$height.'">';
 $col2Index+=1;
 }
 else {
 $col2Index = -1;
 }
 }
 ?>
 </div>
 <div id="col3">
 <?php
 if($col3Index != -1) {
 $name = $columns[3][$col3Index]->name;
 if($name != "") {
 $full_path = $columns[3][$col3Index]->full_path;
 echo '<img src="'.$full_path.'" alt="'.$name.'" width="'.$width.'" height="'.$height.'">';
 $col3Index+=1;
 }
 else {
 $col3Index = -1;
 }
 }
 ?>
 </div>
 <div id="col4">
 <?php
 if($col4Index != -1) {
 $name = $columns[4][$col4Index]->name;
 if($name != "") {
 $full_path = $columns[4][$col4Index]->full_path;
 echo '<img src="'.$full_path.'" alt="'.$name.'" width="'.$width.'" height="'.$height.'">';
 $col4Index+=1;
 }
 else {
 $col4Index = -1;
 }
 }
 ?>
 </div>
 <div id="col5">
 <?php
 if($col5Index != -1) {
 $name = $columns[5][$col5Index]->name;
 if($name != "") {
 $full_path = $columns[5][$col5Index]->full_path;
 echo '<img src="'.$full_path.'" alt="'.$name.'" width="'.$width.'" height="'.$height.'">';
 $col5Index++;
 }
 else {
 $col5Index = -1;
 }
 }
 ?>
 </div>
 </div>
<?php } ?>

$cards has the following info in it:

$cards[$i]['id']
$cards[$i]['lang']
$cards[$i]['name']
$cards[$i]['cost']
$cards[$i]['type']
$cards[$i]['set']
$cards[$i]['rarity']
$cards[$i]['power']
$cards[$i]['toughness']
$cards[$i]['rules']
$cards[$i]['printedname']
$cards[$i]['printedtype']
$cards[$i]['printedrules']
$cards[$i]['flavor']
$cards[$i]['cardnum']
$cards[$i]['artist']
$cards[$i]['sets']
$cards[$i]['rulings']

• Sverri M. Olsen: I think you need to stop for a moment and collect your thoughts. What exactly is it you want to optimize/improve? Is it slow? Is the code getting unwieldy?
• Howard: Starting from $colors = array("B", "R", "G", "W", "U"); where the heart of the code is, I'm trying to figure out if it could be fundamentally better, as in improvement in speed and legibility.
• Howard: @WouterHuysentruit: Would be nice if there was a button that would move it to there.

Answer 1

Unnecessary strcmp

Part of your code looks like this:

if(strcmp($colors[$j], $current_color) != 0) {

strcmp is not necessary here; plain equality would work fine and be clearer:

if($current_color !== $colors[$j]) {

Repetition in Output

I notice you're repeating a lot of code when it comes to output. That should be a clear sign that a loop might be more appropriate. As is, if we were to interpret your $columns array as rows and output something like so:

foreach($columns as $column) {
 foreach($column as $cell) {
 echo "[]";
 }
 echo "\n";
}

it might look like this:

[] [] [] [] [] [] [] [] [] []
[] [] [] [] [] [] []
[] [] [] [] [] [] [] []
[] [] [] []
[] [] [] [] [] [] []
[] [] [] [] [] [] [] []

It would be much easier for us to output it if we could iterate over it the other way, if it were transposed:

[] [] [] [] [] []
[] [] [] [] [] []
[] [] [] [] [] []
[] [] [] [] [] []
[] [] [] [] []
[] [] [] [] []
[] [] [] [] []
[] [] []
[]
[]

Fortunately, that's rather easy with any of the transpose functions here. Once you do that, your repeated code can suddenly be condensed into this much clearer code:

$rows = transpose($columns);
foreach($rows as $row):
?>
 <div class="row">
 <?php for($columnIndex = 0; $columnIndex < 6; $columnIndex++): ?>
 <div class="col<?php echo $columnIndex; ?>">
 <?php
 $card = $row[$columnIndex];
 if(isset($card)):
 ?>
 <!-- snip -->
 <?php endif; ?>
 </div>
 <?php endforeach; ?>
 </div>
<?php
endforeach;

Note also that I've changed your ids to classes. ids must be unique, but you're using many id="row" and id="colN" in a loop, rendering your document invalid. This is the perfect use case for class.

Taking this further, your CSS rules for each column are the same. Rather than having separate col0, col1, ... classes, you could instead just have a column class and apply it to all of them.

• Howard: I didn't really use this but I thought the transpose function was quite clever. Thank you for sharing.
• Pinoniq: Good answer. But, there is so much more wrong with his code. For instance the 'CardData' class. Why use a class here with all the overhead when it's just a glorified array / stdClass instance? Not to speak about all those DRY-violations. So +1 for good answer, but -1 for incomplete answer
• icktoofay: @Pinoniq: You're right that it's incomplete. I wasn't aiming to make it complete; I just listed one main concern (the code duplication) and a few other concerns. I really wasn't expecting this to be the only answer; I was really hoping that there would be another answer to pick up where I left off.