2
\$\begingroup\$

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 && $widthTargetWidthComparison && ($heightCalc <= $target_height )) {
        $new_width = $target_width;
        $new_height = floor( $height * ($target_width / $width) );
    } elseif ($widthHeightComparison && $widthTargetWidthComparison && ($heightCalc > $target_height )) {
        $new_width = floor( $width * ($target_height / $height) );
        $new_height = $target_height;
    } elseif ($widthHeightComparison && !$heightTargetHeightComparison) {
        $new_width = $width;
        $new_height = $height;
    } elseif($widthHeightComparison && $heightTargetHeightComparison) {
        $new_width = floor( $width * ($target_height / $height) );
        $new_height = $target_height;
    } elseif (!$widthHeightComparison && $heightGTETargetHeight){
        $new_width = floor( $width * ($target_height / $height) );
        $new_height = $target_height;
    } elseif (!$widthTargetWidthComparison && !$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) && $str < 0 && $str == round($str));
}

/**
 * Check if expression is positive integer.
 *
 * @param $str
 * @return bool
 */
private function is_positive($str) {
  return (is_numeric($str) && $str > 0 && $str == round($str));
}
\$\endgroup\$
2
\$\begingroup\$

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 > $thumbHeight) {
    $imageWidth  = round($thumbHeight*$imageWidth/$imageHeight);
    $imageHeight = $thumbHeight;
  }
  // return array with result
  return array('width' => $imageWidth,'height' => $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.

|improve this answer|||||
\$\endgroup\$
  • \$\begingroup\$ Maybe something I should have stated from the beginning -- I'm not simply making a square thumbnail from a larger image -- that would be pretty easy and I wouldn't need an algorithm to do it. The purpose of this code is to get the image to fit inside a specialized area on the site, with specific height/width constraints (both maximum & minimum constraints). I could explain it in more detail, but I think that is beside the point of this discussion. \$\endgroup\$ – aaronmcavinue Apr 20 '15 at 12:37
  • \$\begingroup\$ Oh no, a detailed explanation is certainly not beside the point. On Code Review we review your complete code, and don't just give simple answers. I noted that you don't have minimum constraints, as fas a I can see, and I actually tested your routine. My routine does a better job when it comes to mimima. I know it can be hard to believe that something so simple can do a better job than your complex code, but I think it is true. Just try it. I mean, put it to the test. \$\endgroup\$ – KIKO Software Apr 20 '15 at 12:52
  • \$\begingroup\$ I did try it, and it doesn't work. There has to be this many checks, because the image could be either portrait or landscape view. If it is landscape view, it can be resized in a different way than the portrait view, in order to fit into the browser window. "1 = 7, 2 = 4 = 5 and 3 = 6" -- This is wrong. I just wrote the logic out, and 3 != 6... Both this post, and your comment reek of arrogance btw, and are not constructive... \$\endgroup\$ – aaronmcavinue Apr 20 '15 at 12:56
  • \$\begingroup\$ I will give you that 3 & 6 both give the same result, but that doesn't mean the conditionals are the same. Here is the longhand logic for 3: ($width >= $height && $width < $target_width && $height <= $target_height), and here it is for 6: ($width < $height && $height < $target_height && $width <= $target_width) -- If you can combine those into one conditional, please by all means be my guest. \$\endgroup\$ – aaronmcavinue Apr 20 '15 at 13:07
  • \$\begingroup\$ I am sorry if I came over too arrogant. That was not my intention. Critical, yes, arrogant no. I might be missing all the finer details of the English language, because it is not my native tongue. Also, you can read a lot into text that isn't really there. I am really trying to be helpful, no more, no less. What I don't understand is that you say; "It doesn't work.", without explaining what the problem is. Is there a syntax error? I've taken portrait and landschape into account. Combining two if's can be done this way: if ((<condition 1>) || (<condition 2>)) {. \$\endgroup\$ – KIKO Software Apr 20 '15 at 14:02
1
\$\begingroup\$

As well as

$heightCalc = floor($height * ($target_width / $width));

I'd have a

$widthCalc = floor($width * ($target_height / $height));

to avoid repeating the same piece of code several times in the if/else checks.

I'd also consider nesting at least some of the conditions. Maybe something like this:

if ($widthHeightComparison) {
    if ($widthTargetWidthComparison) {
        if ($heightCalc <= $target_height) {
            $new_width = $target_width;
            $new_height = $heightCalc;
        } else {
            $new_width = $widthCalc;
            $new_height = $target_height;
        }
    } elseif ($heightTargetHeightComparison) {
        $new_width = $widthCalc;
        $new_height = $target_height;
    } else {
        $new_width = $width;
        $new_height = $height;
    }
} elseif ($heightGTETargetHeight){
    $new_width = $widthCalc;
    $new_height = $target_height;
} elseif (!$widthTargetWidthComparison && !$heightGTETargetHeight) {
    $new_width = $width;
    $new_height = $height;
} else {
    $new_width = $target_width;
    $new_height = $heightCalc;
}

I'd also consider setting the variable names to one convention - camelcase of underscore -, not both. Even though many editors help you fill in your variable names I feel it's more comfortable not to have to figure out whether it's $new_width or $newWidth, $height_calc or $heightCalc, etc.

|improve this answer|||||
\$\endgroup\$
  • \$\begingroup\$ So, you're in favor of having a bunch of variables declared at the top of the function? Note that this uses up more space in the memory, and the CPU has to do more calculations, whether or not those conditionals/variables are actually used. \$\endgroup\$ – aaronmcavinue Apr 20 '15 at 11:30
  • \$\begingroup\$ I also have a question about the way you've nested the conditionals -- this is actually similar to how I originally had them written, but decided to refactor them into the form you see above because I thought the original way looked more amateurish. Do you have thoughts on this? \$\endgroup\$ – aaronmcavinue Apr 20 '15 at 11:56
  • \$\begingroup\$ I'd drop the declarations of the boolean variables such as $widthHeightComparison, because !is_negative(gmp_cmp($width, $height)) seems more transparent... at least, once I get to know what exactly the gmp_cmp function returns. :P \$\endgroup\$ – dgstranz Apr 20 '15 at 13:18
  • \$\begingroup\$ So I'm glad you mentioned that specific point. Would it be cleaner not to use that gmp_cmp() function at all? Because just like you did, most devs would have to go look it up in the documentation. \$\endgroup\$ – aaronmcavinue Apr 20 '15 at 13:33
  • \$\begingroup\$ Another reason is that I feel I'm repeating code when using an expression several times, as in floor( $width * ($target_height / $height) ), but not necessarily when I call the same function several times, as in !is_negative(gmp_cmp($width, $height)). And I probably wouldn't create a function with 3 inputs just to get $heightCalc and $widthCalc - some would say that two inputs is company and three's a crowd. \$\endgroup\$ – dgstranz Apr 20 '15 at 13:37
1
\$\begingroup\$

Okay, so I updated the code based on the suggestions in the answers. I've reduced the inputs to two arrays, changed the logic of the function completely, changed some naming issues, and gave more explanatory comments. Thanks to KIKO Software and user3206227 for the help.

<?php
/**
 * Resize the thumbnain while maintaining aspect ratio
 * to fit on the homepage. Must fit within width/height
 * constraints found in dimensions.txt, and passed in the
 * $thumb array.
 *
 * @param  array $original, array $thumb
 * @return array
 */
private function calculateThumbnailSize($original, $thumb) {
    // Resize to the thumbnail width and change the height to keep the aspect-ratio.
    $original[height] = floor( ($thumb[width] * $original[height]) / $original[width] );
    $original[width]  = $thumb[width]

    // The image can now be higher than allowed, so shrink if needed.
    if ($original[height] > $thumb[height]) {
        $original[width]  = floor( ($thumb[height] * $original[width]) / $original[height] );
        $original[height] = $thumb[height];
    }
    return array('width' => $original[width],'height' => $original[height];
}

/**
 * Check if expression is negative integer.
 * Created because php doesn't have an built-in
 * function for this.
 *
 * @param $str
 * @return bool
 */
private function isNegative($str) {
  return (is_numeric($str) && $str < 0 && $str == round($str));
}

/**
 * Check if expression is positive integer.
 * Created because php doesn't have an built-in
 * function for this.
 *
 * @param $str
 * @return bool
 */
private function isPositive($str) {
  return (is_numeric($str) && $str > 0 && $str == round($str));
}
\$\endgroup\$
  • \$\begingroup\$ Why did you keep the isNegative() and isPositive() methods? They don't do what you would expect, given their names. May I suggest new names: isNegativeInteger() and isPositiveInteger()?. Mathematically you are correct in excluding zero from both functions, however, I still am quite unhappy with the fact that !isNegative() is not the same as isPositive(). I will have to get used to that. :-) \$\endgroup\$ – KIKO Software Apr 21 '15 at 11:40

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service, privacy policy and cookie policy

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