Dimension comparisons for calculating image thumbnail size I've been toying around with some code I wrote a while back, in an attempt to make it "cleaner" based on what I've read. Does anything stand out to you as bad conventions here? What are your thoughts about doing some conditional checks as separate variables at the top of the function? I thought of putting those into the if/elseif statements, but I thought this method would be DRYer.

/**
 * Calculate the dimensions for the thumbnails.
 *
 * @param  $width, $height, $target_width, $target_height
 * @return array
 */
private function calculateThumbnailSize($width, $height, $target_width, $target_height) {
    $widthHeightComparison = !is_negative(gmp_cmp($width, $height));
    $widthTargetWidthComparison = !is_negative(gmp_cmp($width, $target_width));
    $heightTargetHeightComparison = is_positive(gmp_cmp($height, $target_height));
    $heightGTETargetHeight = !is_negative(gmp_cmp($height, $target_height));
    $heightCalc = floor($height * ($target_width / $width));

    if ($widthHeightComparison &amp;&amp; $widthTargetWidthComparison &amp;&amp; ($heightCalc &lt;= $target_height )) {
        $new_width = $target_width;
        $new_height = floor( $height * ($target_width / $width) );
    } elseif ($widthHeightComparison &amp;&amp; $widthTargetWidthComparison &amp;&amp; ($heightCalc &gt; $target_height )) {
        $new_width = floor( $width * ($target_height / $height) );
        $new_height = $target_height;
    } elseif ($widthHeightComparison &amp;&amp; !$heightTargetHeightComparison) {
        $new_width = $width;
        $new_height = $height;
    } elseif($widthHeightComparison &amp;&amp; $heightTargetHeightComparison) {
        $new_width = floor( $width * ($target_height / $height) );
        $new_height = $target_height;
    } elseif (!$widthHeightComparison &amp;&amp; $heightGTETargetHeight){
        $new_width = floor( $width * ($target_height / $height) );
        $new_height = $target_height;
    } elseif (!$widthTargetWidthComparison &amp;&amp; !$heightGTETargetHeight) {
        $new_width = $width;
        $new_height = $height;
    } else {
        $new_width = $target_width;
        $new_height = floor( $height * ($target_width / $width) );
    }
    return array($new_width, $new_height);
}

/**
 * Check if expression is negative integer.
 *
 * @param $str
 * @return bool
 */
private function is_negative($str) {
  return (is_numeric($str) &amp;&amp; $str &lt; 0 &amp;&amp; $str == round($str));
}

/**
 * Check if expression is positive integer.
 *
 * @param $str
 * @return bool
 */
private function is_positive($str) {
  return (is_numeric($str) &amp;&amp; $str &gt; 0 &amp;&amp; $str == round($str));
}
Trial and Error programming?

My first feeling is that is poorly written code. Why? It is too complex. Calculating the dimensions of a thumbnail should be an easy task, but this code manages to make it into something that has to be studied for an hour before it can be decrypted.

You use !is_negative() but also is_positive(). Naively I would say: "But those are the same!". But, of course, they are not.

I could live with the fact you feel the need to invoke the GMP library, but really? Nobody has images that big, nor do you use it in places where it matters. You don't even use the three values of the outcome of gmp_cmp(), you only use two.

Since you haven't told us, I start by assuming that your function will always be fed by valid numbers. Normal numbers. If this assumption is wrong you can add an is_numeric() error check, or a conversion at the beginning.

Your variables names are poorly chosen. $widthHeightComparison should be something like $imageIsWiderThanHigh or $imageIsLandscape. Same is true for the other variables: $heightCalc doesn't mean anything.

All your function does is calculate a new width and height, based on four parameters: The width and height of the image and the maximum height and width of the thumbnail.

You've decided that there are seven possible ways in which this must be calculated. Let me number them 1 to 7, from top to bottom. To my surprise they are not all different: 1 = 7, 2 = 4 = 5 and 3 = 6. So really there are only three ways in which you perform your caculation. Did you know this? Clearly not.

I think this code was written, not by thinking about the problem, but by random trial and error. You simply gave it some numbers and every time the outcome wasn't what you expected, you added something to correct that, until, after a couple of hours, it passed all the tests. I guess you could say that this code is the result of Test-driven development. Clearly that is not a good way to approach this problem. I prefer it when somebody thinks about the task at hand before writing down code. Afterall, this is not a complex problem.

So here my first stab at this:

function thumbnailDimensions($imageWidth,$imageHeight,$thumbWidth,$thumbHeight)
// resize to thumbnail while maintaining aspect-ratio
{
  // we resize to the thumbnail width and change the height to keep the aspect-ratio
  $imageHeight = round($thumbWidth*$imageHeight/$imageWidth);
  $imageWidth  = $thumbWidth;
  // the image can now be higher than allowed, so we shrink if needed
  if ($imageHeight &gt; $thumbHeight) {
    $imageWidth  = round($thumbHeight*$imageWidth/$imageHeight);
    $imageHeight = $thumbHeight;
  }
  // return array with result
  return array('width' =&gt; $imageWidth,'height' =&gt; $imageHeight);
}


So that's all that is needed, as far as I can tell. You see, this is not rocket science. 

The answer to your question is: No local variables are needed, but if you do use them, give them proper names. Yes, I am in favor of using local variables if it means you don't have to compute something multiple times.