current community

your communities

more stack exchange communities

Stack Exchange
tour help
Take the 2-minute tour ×
Code Review Stack Exchange is a question and answer site for peer programmer code reviews. It's 100% free, no registration required.
up vote 5 down vote favorite

This is my code:

CSS code:

#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 code:

<?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 } ?>

The above code is displaying 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 the following xml file: http://ark42.com/mtg/gathererdownloader/MTGCardInfo/

$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']

Any suggestions?

share|improve this question
    
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? –  Sverri M. Olsen Aug 18 '13 at 4:57
    
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. –  rotaercz Aug 18 '13 at 5:00
    
@WouterHuysentruit: Would be nice if there was a button that would move it to there. –  rotaercz Aug 18 '13 at 5:01

1 Answer 1

up vote 5 down vote accepted

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.

share|improve this answer
    
I didn't really use this but I thought the transpose function was quite clever. Thank you for sharing. –  rotaercz Aug 19 '13 at 4:00
    
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 –  Pinoniq Aug 20 '13 at 7:03
2  
@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. –  icktoofay Aug 21 '13 at 3:54

Your Answer

 
discard

By posting your answer, you agree to the privacy policy and terms of service.

Not the answer you're looking for? Browse other questions tagged or ask your own question.