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

Question

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]; 
                }
            }                  
        }
    }  

Answer 1

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.

Answer 2

One of the points that should raise a query is the amount of repeated code - you have a duplicate for every if. This suggests creating a function to handle it instead. Personally, I would end up with something like the following:

function is_better_image( $image, $thumbnail_width, $thumbnail_height )
{
    $image_dimensions = getimagesize($image);
    if ( ( $image_dimensions[0] > $thumbnail_width ) && ( $image_dimensions[1] > $thumbnail_height ) ) {
        return true; 
    }
    return false;
}

function check_image( $image, $width, $height )
{
    if ( !empty( $image ) ) {
        if ( is_better_image( $image, $width, $height ) ) {
            return $image;
        }
    }
}
...
if ( isset( $matches ) ) {
    // check all possible image matches
    $possible_thumbnail_img = check_image( $matches[1][0], $width, $height );
    if ( is_null( $possible_thumbnail_img ) ) {
        $possible_thumbnail_img = check_image( $matches[1][1], $width, $height );
    }
    if ( is_null( $possible_thumbnail_img ) ) {
        $possible_thumbnail_img = check_image( $matches[1][2], $width, $height );
    }
    // if we have a suitable image, use that instead
    if ( !is_null( $possible_thumbnail_img ) ) {
        $thumbnail_img = $possible_thumbnail_img
    }
}