This file takes a user picture upload and converts two pictures which are named according to the Session ID.

Right now he biggest drawback is that you cannot crop the image at upload time like you can on popular sites like Facebook.

However I'm just looking for overall constructive criticism on the structure of the class. The paths to save the files to are constants.

I thought about breaking it up in to two classes but I want to stick with just one.


 *      Module  :       Model
 *      Name    :       Upload
 *      Input   :       File Information
 *      Output  :       Resized Files in .jpg format
 *      Notes   :

 resizeMove() - resizes the picture to $maxMedium and $maxSmall and moves the file to a permanent location.  
 makeDimensions() - calculates the dimensions of the new images so that there is not distortion if possible.
 getImage() - creates a php image for manipulation.
 updateSessionAndDb - updates the mysql table - move out.


    class Upload
        private $originalWidth, 
                $maxMedium = 50,
                $maxSmall = 20;

        private $src = NULL;


        function __construct($fileType, $fileName)

            $this->sessionId = Session::get('id');
            $this->path_medium = Constant::PICTURES . "$this->sessionId.jpg";
            $this->path_small = Constant::PICTURES . "$this->sessionId-1.jpg";
            $this->fileType = $fileType;
            $this->fileName = $fileName;


        private function createImages()
            if(move_uploaded_file($this->fileName, $this->path_medium))

        private function resizeMove($max, $path)
            $image_true_color = imagecreatetruecolor($this->newWidth, $this->newHeight);
            imagecopyresampled($image_true_color, $this->src, 0, 0, 0, 0, $this->newWidth, $this->newHeight, $this->
            originalWidth, $this->originalHeight);
            imagejpeg($image_true_color, $path);

        private function makeDimensions($max)
            if(($this->originalWidth > $this->originalHeight) && ($this->originalWidth > $max))
                $this->newWidth = $max;
                $this->newHeight = ($max / $this->originalWidth) * $this->originalHeight;
            elseif($this->originalHeight > $this->originalWidth && $this->originalHeight > $max)
                $this->newHeight = $max;
                $this->newWidth = ($max / $this->originalHeight) * $this->originalWidth;
            elseif ($this->originalWidth > $max)
                $this->newWidth = $this->newHeight = $max;

        private function getImage($path)
            $type_creators = array( 
                'image/gif' => 'imagecreatefromgif', 
                'image/pjpeg' => 'imagecreatefromjpeg', 
                'image/jpeg' => 'imagecreatefromjpeg', 
                'image/png' => 'imagecreatefrompng'); 
            if(array_key_exists($this->fileType, $type_creators)) 
                $this->src = $type_creators[$this->fileType]($path); 
                return true; 
        return false; 

1 Answer 1


With this piece of code you are almost beginning to write good OO. Its good to see you are trying to hide implementation details from other classes.

However, what you have here is a constructor that does everything. The constructor should do no more than initialize the object. Have faith that once you have created your object you can then call its public methods to let the object do what needs to be done.

Think about what an "Upload" object is? The resize move could be a good interace to the rest of the system (that is the essence of uploading)? What else should an "Upload" object be responsible for? Make those things that the object should be responsible for public methods so that other objects can call them.

Updating the session here seems a little odd is that part of uploading?

  • \$\begingroup\$ The 5 assignments I make to my private member variables in the constructor I consider initialization. It is even commented as such in the code...so I'm guessing you don't consider this intitialization....why not? \$\endgroup\$
    – user7459
    Commented Jan 12, 2012 at 18:02
  • \$\begingroup\$ No, those 5 assignments are perfect. Its just the instantiate method that I don't think should exist and be called. \$\endgroup\$
    – Paul
    Commented Jan 13, 2012 at 1:31
  • \$\begingroup\$ I used to do things like that too, but this is not about a matter of preference. By having no callable methods this boils down to a fancy way of calling a function. Look at it closely, the way you have it - it is completely procedural. It is just the equivalent of a function call (and if you are going to write it like that just use a function rather than pretending to have real objects). You are not gaining any of the benefits of OO. The benefit of OO is not to use 1 line where you could use 2. The benefit is breaking the program into components with objects. \$\endgroup\$
    – Paul
    Commented Jan 14, 2012 at 1:43

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service and acknowledge you have read our privacy policy.