1
\$\begingroup\$

I am working on an image manipulation class for a project to re-size uploaded slider images to 780 X 397 and uploaded avatar pictures to 220 X 220. I am wondering if this is a good way to construct it.

What would be the best way to handle an image that is smaller then 780 X 397. Should I simply re-size it or go about it a different way?

I have included the class code as well as the code syntax on how it is to be used:

Class code

class imageEditer{
    private $filename;
    public $picWidth;
    public $picHeight;
    public $picMimeType;
    public $picMimeTypeCons;
    private $allowedMimeType = array(IMAGETYPE_GIF=>'.gif',IMAGETYPE_JPEG=>'.jpg',IMAGETYPE_PNG=>'.png',IMAGETYPE_BMP=>'.bmp');
    public $picture;
    private $imageWork;
    private $storePath;

    public function __construct($filename){
        if(is_file($filename)){
            $this->filename = $filename;
            $getimagesize = getimagesize($this->filename);
            $this->picWidth = $getimagesize[0];
            $this->picHeight = $getimagesize[1];
            $this->picMimeTypeCons = $getimagesize[2];
            if($this->allowedMime()){
                $this->picMimeType = image_type_to_mime_type($this->picMimeTypeCons);
                $this->picture = $this->imageCreate($this->picMimeTypeCons);
            }else{
                trigger_error('Mime type not allowed',E_USER_ERROR);
            }
        }else{
            trigger_error('File not found',E_USER_ERROR);
        }
    }

    public function imageCreate($mimeTypeCons){
        switch ($mimeTypeCons) {
            case IMAGETYPE_GIF:
                return imagecreatefromgif($this->filename);
                break;
            case IMAGETYPE_JPEG:
                return imagecreatefromjpeg($this->filename);
                break;
            case IMAGETYPE_PNG:
                return imagecreatefrompng($this->filename);
                break;
            case IMAGETYPE_BMP:
                return imagecreatefromwbmp($this->filename);
                break;
            default:
                trigger_error("Couldn't create image.",E_USER_ERROR);
                break;
        }
    }

    public function imageCreateSlider($storePath,$width = 780,$height = 397){
        $this->storePath = $storePath;
        $divWidth = floor(($this->picWidth-$width)/2) ;
        $divHeight = floor(($this->picHeight-$height)/2);
        $this->imageWork = imagecrop($this->picture,array("x"=>$divWidth,"y"=>$divHeight,"width"=>$width,"height"=>$height));
        if(strstr(basename($this->storePath),'.') != strstr(basename($this->filename),'.')){
            while ($fileEx = current($this->allowedMimeType)) {
                if(strstr(basename($this->storePath),'.') == $fileEx){
                   $this->imageCreateType(key($this->allowedMimeType));
                    return;
                }
                next($this->allowedMimeType);
            }
        }else{
            $this->imageCreateType($this->picMimeTypeCons);
        }
    }

    public function imageAvatar($storePath,$width = 220,$height = 220){
        $this->storePath = $storePath;
        $this->imageWork = imagescale($this->picture,$width,$height);
        if(strstr(basename($this->storePath),'.') != strstr(basename($this->filename),'.')){
            while ($fileEx = current($this->allowedMimeType)) {
                if(strstr(basename($this->storePath),'.') == $fileEx){
                   $this->imageCreateType(key($this->allowedMimeType));
                    return;
                }
                next($this->allowedMimeType);
            }
        }else{
            $this->imageCreateType($this->picMimeTypeCons);
        }
    }

    public function imageCreateType($mimeTypeCons){
        switch ($mimeTypeCons) {
            case IMAGETYPE_GIF:
                return imagegif($this->imageWork,$this->storePath);
                break;
            case IMAGETYPE_JPEG:
                return imagejpeg($this->imageWork,$this->storePath);
                break;
            case IMAGETYPE_PNG:
                return imagepng($this->imageWork,$this->storePath,9,PNG_ALL_FILTERS);
                break;
            case IMAGETYPE_BMP:
                return image2wbmp($this->imageWork,$this->storePath);
                break;
            default:
                trigger_error("Couldn't create image.",E_USER_ERROR);
                break;
        }
    }

    public function allowedMime(){
        for ($i=0; $i <count($this->allowedMimeType) ; $i++) {
            return array_key_exists($this->picMimeTypeCons,$this->allowedMimeType);
        }
    }
    public function __destruct(){
        imagedestroy($this->picture);
        if(!empty($this->imageWork)) imagedestroy($this->imageWork);
    }
}

Code syntax

$imageEditer->imageCreateSlider('images/slider01.png');
$imageEditer->imageAvatar('image/useravatar.png);
\$\endgroup\$
  • \$\begingroup\$ The good practice will be to resize it proportional. If it is smaller than 220x220 then add extra background. \$\endgroup\$ – Adil Aliyev Nov 14 '14 at 10:36
  • \$\begingroup\$ It seems your class already exists here. Quite some work has gone to it, and it could help you figure something out. \$\endgroup\$ – samayo Nov 14 '14 at 11:40
1
\$\begingroup\$
class ImageEditor {

The general convention is to make classes PascalCase, so ImageEditor would be the recommended name.

static private $allowedMimeTypes = array(IMAGETYPE_GIF => '.gif', IMAGETYPE_JPEG => '.jpg', IMAGETYPE_PNG => '.png', IMAGETYPE_BMP => '.bmp');

I'd call this $allowedMimeTypes, as there's more than one. Alternately, you could name it $extensionOf and use it to mean that $extensionOf[IMAGETYPE_GIF] is .gif.

I changed this to a static variable as it's the same for every object. We only need one copy, not one per object. This changes the usage slightly. We'll access as ImageEditor::allowedMimeTypes.

It's generally easier to read if you add a little whitespace between elements.

public function __construct($filename){
    if ( ! is_file($filename) ) {
        trigger_error('File not found', E_USER_ERROR);
        return;
    }

You can avoid unnecessary nesting by returning on an error rather than putting the whole function body inside an if/else block. Because of the return, the rest of the function is implicitly an else, but you don't have to write that out now.

    $this->filename = $filename;

    $getimagesize = getimagesize($this->filename);
    $this->picWidth = $getimagesize[0];
    $this->picHeight = $getimagesize[1];
    $this->picMimeTypeCons = $getimagesize[2];

    if ( ImageEditor::isAllowedMime($this->picMimeTypeCons) ) {
        $this->picMimeType = image_type_to_mime_type($this->picMimeTypeCons);
        $this->picture = $this->imageCreate($this->picMimeTypeCons);
    } else {
        trigger_error('Mime type not allowed',E_USER_ERROR);
    }
}

Separating statements into groups with whitespace can make it easier to see what goes with what.

The first time that I read this, I didn't understand that the allowedMime() function relied on $this->picMimeTypeCons being set. Refactoring so that that is a static class function rather than an object function would allow for it to be externally called without an object. More importantly, we now call it ImageEditor::isAllowedMime($this->picMimeTypeCons) which makes it obvious what it requires.

I changed the name from allowedMime to isAllowedMime to indicate that it returns a boolean value.

        default:
            trigger_error("Couldn't create image.", E_USER_ERROR);
            break;
    }

While putting the explicit break there won't hurt anything, it's not necessary. You can just let the block end and it will fall out naturally. You only need the explicit break when the following code should not run under that case. You're out of cases here, so it's not needed.

    if ( strstr(basename($this->storePath),'.') == strstr(basename($this->filename),'.') ) {
        $this->imageCreateType($this->picMimeTypeCons);
        return;
    }

    foreach ( ImageEditor::allowedMimeTypes as $type => $fileExt ) {
        if ( strstr(basename($this->storePath),'.') == $fileExt ) {
            $this->imageCreateType($type);
            return;
        }
    }

Again, I did an early return in the error case.

I switched from the while to a foreach as more readable. The while version was incorrect too. You need to reset($this->allowedMimeType); before using current, next, and key.

I also changed from $this->allowedMimeType to ImageEditor::allowedMimeTypes as previously discussed.

public function allowedMime(){
    for ($i=0; $i <count($this->allowedMimeType) ; $i++) {
        return array_key_exists($this->picMimeTypeCons,$this->allowedMimeType);
    }
}

As a general rule, you shouldn't call functions in loops unless the input changes as you go through the loop. In this case it doesn't, so you could write:

public function allowedMime(){
    for ( $i = 0, $n = count($this->allowedMimeTypes); $i < $n; $i++) {
        return array_key_exists($this->picMimeTypeCons,$this->allowedMimeType);
    }
}

But we don't need to do that, as we have a foreach for iterating over arrays:

public function allowedMime(){
    foreach ( $this->allowedMimeType as $type ) {
        return array_key_exists($this->picMimeTypeCons,$this->allowedMimeType);
    }
}

But we aren't using $type, so we could actually write

public function isAllowedMime($type) {
    return array_key_exists($type, ImageEditor::allowedMimeTypes);
}

Notice that I also changed the name and parameters as previously discussed.

Note that the original loop would only ever run once. It would always return in the first iteration of the loop. Fortunately, array_key_exists does the right thing and doesn't need to be run in a loop.

To address your question, it is risky to resize an image to be bigger. Smaller images often don't have enough detail to be made larger. Sometimes with a high quality JPEG, it will work, as JPEGs can have more detail than they can display. With the bitmap based formats (GIF, PNG, BMP), the code has to sort of guess as to how to expand things. It's generally more reliable to make images smaller than larger.

As Adil Aliyev's comment notes, it is also better to keep the image proportional. I.e. don't try to make a 220x440 image into a 220x220. It will look squashed. But a 440x440 or even a 397x397 will resize better. It can help to either crop or extend the original image to make it have a proportional ratio before resizing. Extending is more reliable.

That kind of question is a bit off-topic for Code Review though. You might find that you get better answers on Stack Overflow or another source.

\$\endgroup\$
  • \$\begingroup\$ Thank you very much for your answer it was very helpful. \$\endgroup\$ – andrewnite Nov 17 '14 at 8:50

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.