HiveBrain v1.2.0
Get Started
← Back to all entries
patternphpMinor

Displaying images based on column colors

Submitted by: @import:stackexchange-codereview··
0
Viewed 0 times
columndisplayingcolorsbasedimages

Problem

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

```
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; $jcost, $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


name;
if($name != "") {
$full_path = $columns[0][$col0Index]->full_path;
echo '';
$col0Index+=1;
}
else {
$col0Index = -1;
}
}
?>



name;
if($name != "") {
$full_path = $columns[1][$col1Index]->full_path;
echo '';
$col1Index+=1;
}
else {
$col1Index = -1;
}
}
?>



name;
if($name != "") {
$full_path = $c

Solution

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):
?>
    
    
        ">
        
            
        
        
    
    
<?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.

Code Snippets

if(strcmp($colors[$j], $current_color) != 0) {
if($current_color !== $colors[$j]) {
foreach($columns as $column) {
    foreach($column as $cell) {
        echo "[]";
    }
    echo "\n";
}
[] [] [] [] [] [] [] [] [] []
[] [] [] [] [] [] []
[] [] [] [] [] [] [] []
[] [] [] []
[] [] [] [] [] [] []
[] [] [] [] [] [] [] []
[] [] [] [] [] []
[] [] [] [] [] []
[] [] [] [] [] []
[] [] [] [] [] []
[] [] []    [] []
[] [] []    [] []
[] [] []    [] []
[]    []       []
[]
[]

Context

StackExchange Code Review Q#29908, answer score: 6

Revisions (0)

No revisions yet.