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

If any image found and bigger than thumbnail settings, get that one

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

Problem

Is this an ideal use of if..else or can this code be simplified/beautified further?

// If any image found and bigger than thumbnail settings, get that one
    if ( isset( $matches ) && !empty( $matches[1][0] ) ) {
        $image_found = $matches[1][0]; 
        $image_dimensions = getimagesize($image_found);
        if ( ( $image_dimensions[0] > $width ) && ( $image_dimensions[1] > $height ) ) {
            $thumbnail_img = $matches[1][0]; 
        }
        // Check 2nd image found
        else if ( !empty( $matches[1][1] ) ) {
            $image_found = $matches[1][1]; 
            $image_dimensions = getimagesize($image_found);
            if ( ( $image_dimensions[0] > $width ) && ( $image_dimensions[1] > $height ) ) {
                $thumbnail_img = $matches[1][1]; 
            } 
            // Check 3rd image found    
            else if ( !empty( $matches[1][2] ) ) {
                $image_found = $matches[1][2]; 
                $image_dimensions = getimagesize($image_found);
                if ( ( $image_dimensions[0] > $width ) && ( $image_dimensions[1] > $height ) ) {
                    $thumbnail_img = $matches[1][2]; 
                }
            }                  
        }
    }

Solution

Let me make it clear for myself. You have an array of images, which sometimes can be empty. You start off by checking the first image, if it's valid for your needs you take it, and if not you keep on checking the next one, and the next one, and so on until you find the one which is valid. Is that right? If so, here are my few thoughts.

Firstly, I must say that Glenn has made good points, namely: 1) factor the common code in all cases out to a separate function, and 2) if you have nested conditionals, most times it's better and more readable to replace it with Guard Clauses.
However, personally I would prefer the following solution:

$theImg = null;
$i = 0;
while (!empty($matches[1][$i])) {
    $imgAtHand = $matches[1][$i];
    $dimensions = getimagesize($imgAtHand);
    if (($dimensions[0] > $width) && ($dimensions[1] > $height)) {
        $theImg = $imgAtHand;
        break;
    }
    $i++;
}


It has a couple of advantages over the "nested conditional" approach:

  • it eliminates code duplication;



  • it will work regardless of the array size.



One more thing. The following piece of code

isset($matches) && !empty($matches[1][0])


may well be replaced with

!empty($matches[1][0])


to make the code less verbose.

Code Snippets

$theImg = null;
$i = 0;
while (!empty($matches[1][$i])) {
    $imgAtHand = $matches[1][$i];
    $dimensions = getimagesize($imgAtHand);
    if (($dimensions[0] > $width) && ($dimensions[1] > $height)) {
        $theImg = $imgAtHand;
        break;
    }
    $i++;
}
isset($matches) && !empty($matches[1][0])
!empty($matches[1][0])

Context

StackExchange Code Review Q#23412, answer score: 6

Revisions (0)

No revisions yet.