1
\$\begingroup\$

I've combined some of my own functions with a function I found and I'm curious to know what others think of it. It will be used to upload and resize/crop images that are jpeg, jpg, png.

<?php
class ImageUploader {
    var $multiple_file_upload = true;
    var $max_size = 1024;
    var $mime_types = array('bmp' => 'image/bmp', 'gif' => 'image/gif', 'jpeg' => 'image/jpeg', 'jpg' => 'image/jpeg', 'png' => 'image/png');
    var $allowed_extensions;
    function __construct() {

    }

    function get_image_information($file) {
        return getimagesize($file);
    }

    function get_image_extension($file) {
        return pathinfo($file, PATHINFO_EXTENSION);
    }

    function resize_image($source_image, $destination_filename, $width = 100, $height = 100, $quality = 70, $crop = false) {

        $image_data = $this -> get_image_information($source_image);

        switch( $image_data['mime'] ) {
            case 'image/gif' :
                $get_func = 'imagecreatefromgif';
                $suffix = ".gif";
                break;
            case 'image/jpeg' :
                $get_func = 'imagecreatefromjpeg';
                $suffix = ".jpg";
                break;
            case 'image/png' :
                $get_func = 'imagecreatefrompng';
                $suffix = ".png";
                break;
        }

        $img_original = call_user_func($get_func, $source_image);
        $old_width = $image_data[0];
        $old_height = $image_data[1];
        $new_width = $width;
        $new_height = $height;
        $src_x = 0;
        $src_y = 0;
        $current_ratio = round($old_width / $old_height, 2);
        $desired_ratio_after = round($width / $height, 2);
        $desired_ratio_before = round($height / $width, 2);

        if ($old_width < $width || $old_height < $height) {
            /**
             * The desired image size is bigger than the original image.
             * Best not to do anything at all really.
             */
            return false;
        }

        /**
         * If the crop option is left on, it will take an image and best fit it
         * so it will always come out the exact specified size.
         */
        if ($crop) {
            /**
             * create empty image of the specified size
             */
            $new_image = imagecreatetruecolor($width, $height);

            /**
             * Landscape Image
             */
            if ($current_ratio > $desired_ratio_after) {
                $new_width = $old_width * $height / $old_height;
            }

            /**
             * Nearly square ratio image.
             */
            if ($current_ratio > $desired_ratio_before && $current_ratio < $desired_ratio_after) {
                if ($old_width > $old_height) {
                    $new_height = max($width, $height);
                    $new_width = $old_width * $new_height / $old_height;
                } else {
                    $new_height = $old_height * $width / $old_width;
                }
            }

            /**
             * Portrait sized image
             */
            if ($current_ratio < $desired_ratio_before) {
                $new_height = $old_height * $width / $old_width;
            }

            /**
             * Find out the ratio of the original photo to it's new, thumbnail-based size
             * for both the width and the height. It's used to find out where to crop.
             */
            $width_ratio = $old_width / $new_width;
            $height_ratio = $old_height / $new_height;

            /**
             * Calculate where to crop based on the center of the image
             */
            $src_x = floor((($new_width - $width) / 2) * $width_ratio);
            $src_y = round((($new_height - $height) / 2) * $height_ratio);
        }
        /**
         * Don't crop the image, just resize it proportionally
         */
        else {
            if ($old_width > $old_height) {
                $ratio = max($old_width, $old_height) / max($width, $height);
            } else {
                $ratio = max($old_width, $old_height) / min($width, $height);
            }

            $new_width = $old_width / $ratio;
            $new_height = $old_height / $ratio;

            $new_image = imagecreatetruecolor($new_width, $new_height);
        }

        /**
         * Where all the real magic happens
         */
        imagecopyresampled($new_image, $img_original, 0, 0, $src_x, $src_y, $new_width, $new_height, $old_width, $old_height);

        /**
         * Save it as a JPG File with our $destination_filename param.
         */
        imagejpeg($new_image, $destination_filename, $quality);

        /**
         * Destroy the evidence!
         */
        imagedestroy($new_image);
        imagedestroy($img_original);

        /**
         * Return true because it worked and we're happy. Let the dancing commence!
         */
        return true;
    }

}
?>

It works fine but any improvements or suggestions are welcome.

\$\endgroup\$
  • \$\begingroup\$ var is depricated use protected,public or private. \$\endgroup\$ – AlucardTheRipper Apr 10 '13 at 17:32
  • \$\begingroup\$ You shouldn't have member variables when you don't use them in functions. And your functions should also have protected,public or private. \$\endgroup\$ – AlucardTheRipper Apr 10 '13 at 17:43
  • \$\begingroup\$ Skip all useless comments. Just comment the intention and not the code itself. \$\endgroup\$ – mheinzerling Apr 11 '13 at 5:38
1
\$\begingroup\$

Some hints for (not) commenting your code.

Change:

/**
 * Landscape Image
 */
if ($current_ratio > $desired_ratio_after) {

To:

$isLandscape=$current_ratio > $desired_ratio_after;
if ($isLandscape) {

Or:

if ($old_width < $width || $old_height < $height) {
        /**
         * The desired image size is bigger than the original image.
         * Best not to do anything at all really.
         */
        return false;
}

To:

$toSmallToResize = $old_width < $width || $old_height < $height
if ($toSmallToResize) return false;

(Maybe the || should be a && ?)

As I said in the comments, get rid of all comments without new informations for the reader.

You could also move your switch into a separate method with a nice name to reduce the length of your main method. And the crop and resize_only part are also suitable for a private method. (This methods would also be very easily to test in PHPUnit; just a bunch of numbers and no real images required.)

|improve this answer|||||
\$\endgroup\$
0
\$\begingroup\$

There are two ideas mixed together that complicate the code:

  • Manipulating an image.
  • Uploading an image.

Separate the concerns into two different classes (e.g., SimpleImage and SecureImageUpload).

See the SimpleImage class for a reasonably designed class that handles image manipulation. It has methods to resize, scale, load, and save image files of various formats. I'd recommend that you extend the SimpleImage class to include methods that expose the extra functionality you desire.

Encapsulation means that object data is manipulated by exposing methods to other objects. For example:

$image->load( "filename.jpg" );
$image->scale( 100 );
$image->save( "thumbnails/filename.jpg" );

Securely handling image file uploads in PHP is a daunting task to do correctly. See:

|improve this answer|||||
\$\endgroup\$

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.