patternphpMinor
Displaying images based on column colors
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
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
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
Part of your code looks like this:
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
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:
Note also that I've changed your
Taking this further, your CSS rules for each column are the same. Rather than having separate
strcmpPart 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.