3
\$\begingroup\$

Please take it easy on my as this is my first OOP web application I plan on converting it to some deeper degree but not sure yet.

The basic functionality is there I think, but I'm looking for some feedback on how I can improve this in terms of OOP and simplicity. Sorry for any simple mistakes only been doing php for a few months and its my first language :)

Many Thanks

<?php

class Image{

/*
*list of properties 
*image filepath gets pasted in $imageFilepath
*/
public $imageFilepath = "";
public $imageTitle;
public $imageSize;

/*
*list of properties 
*image filepath gets pasted in $imageFilepath
*/
public function __construct() {
     echo 'Welcome to Dannys awesome Gallery';  
}

/*
*function to get the image filepath, runs through the files the outputs then in an array
*
*/

public function getimage_Filepath() {
    $image_files = get_files($imageFilepath);
    if (count($image_files)) {
        $index = 0;
        foreach($image_files as $index=>$file)
        $index++;   
    }
    return;
}

/*
*Outputs the file images found
*
*/

public function setimage_Filepath() {
    echo $this->getimage_Filepath();
}

/*
*Generates the image name, from the actual filename
*
*/

public function getimage_title($imageTitle) {
//gets the image title using basename 
    $imageTitle = $this->imageFilepath;
    basename($imageTitle);
    return $imageTitle;
}

/*
*Outputs the image name 
*
*/

public function setimage_title($imageTitle) {
    //displays the image title
    echo $this->imageTitle;
}

}
$image = new Image;
?>
\$\endgroup\$
1
\$\begingroup\$

The code below is good. I only changed the comments a bit to make them a bit clearer and more concise. However, one glaring issue is that it seems you have your setters and getters mixed up. Note the getimage_*() and setimage_*() functions you have in your class. Usually, setter and getter functions do exactly that: they set a value and get a value. What you seem to be doing is using the set* functions to get the value returned by another function. This is +wrong+.

<?php

class Image{
    public $imageFilepath = "";
    public $imageTitle;
    public $imageSize;

    /**
     * Construct
     */
    public function __construct() {
         echo 'Welcome to Dannys awesome Gallery';  
    }

    /**
     * Get image filepath
     *
     * @return array
     */
    public function getimage_Filepath() {
        $image_files = get_files($imageFilepath);
        if (count($image_files)) {
            $index = 0;
            foreach($image_files as $index=>$file)
            $index++;   
        }
        return;
    }


    /**
     * Output tile image found
     *
     * @return array
     */
    public function setimage_Filepath() {
        echo $this->getimage_Filepath();
    }


    /**
     * Generate image name derived from filename
     *
     * @return string
     */
    public function getimage_title($imageTitle) {
        //gets the image title using basename 
        $imageTitle = $this->imageFilepath;
        basename($imageTitle);
        return $imageTitle;
    }

    /**
     * Output the image name
     *
     * @return string
     */
    public function setimage_title($imageTitle) {
        //displays the image title
        echo $this->imageTitle;
    }
}
?>

Now, let's get rid of the set* functions:

<?php

class Image{
    public $imageFilepath = "";
    public $imageTitle;
    public $imageSize;

    /**
     * Construct
     */
    public function __construct() {
         echo 'Welcome to Dannys awesome Gallery';  
    }

    /**
     * Get image filepath
     *
     * @return array
     */
    public function getimage_Filepath() {
        $image_files = get_files($imageFilepath);
        if (count($image_files)) {
            $index = 0;
            foreach($image_files as $index=>$file)
            $index++;   
        }
        return;
    }


    /**
     * Generate image name derived from filename
     *
     * @return string
     */
    public function getimage_title($imageTitle) {
        //gets the image title using basename 
        $imageTitle = $this->imageFilepath;
        basename($imageTitle);
        return $imageTitle;
    }
}
?>

Great! Looking much better. Now, remember that your construct should be used to set variables or initialize functions that the object requires - NOT echoing something out (unless its an Exception, in which case you'd use throw new Exception('')). So let's move the echo OUT of the class, initialize the $imageFilepath variable:

<?php

class Image{
    private $imageFilepath;
    private $imageTitle;
    private $imageSize;

    /**
     * Construct
     */
    public function __construct(string $imageFilepath) {
        if(empty($imageFilepath)){
            throw new Exception("Expecting image file path, nothing given.");
        } else {
            $this->imageFilepath = $imageFilepath;
        }
    }

    /**
     * Get image filepath
     *
     * @return array
     */
    public function getImageFilepath() {
        return $this->imageFilepath;
    }

    /**
     * Get the files in a directory pointed by imageFilepath
     * 
     * @return array
     */
    public function getFilesInFilepath(){
        $directoryContent = scandir($this->imageFilepath);
        foreach($directoryContent as $key => $value){
            $path = $this->imageFilepath . PATH_SEPARATOR . $value;
            if(is_file($path) && is_readable($path)){
                $files[] = $path;
            }
        }
        return $files;
    }


    /**
     * Generate image name derived from filename
     *
     * @return highlight_string(str)                                                                                                                                                                                                                                                                                                                                                                                                                                                                                                                                                                                                                                                                                                                                                
     */
    public function getImageTitle(string $imageFile) {
        return basename($imageFile);
    }
}

// We initialize the object and set the imageFilepath at the same time
$image = new Image("/home/jsanc623/Pictures/");

// We can get the image filepath
$imageFilepath = $image->getImageFilepath();

// We can get an array of all the files in a directory
$filesInFilePath = $image->getFilesInFilepath();

// We can then get the image titles for each image/
foreach($filesInFilePath as file){
    $imageTitle[] = $image->getImageTitle($file);
}

print_r($imageTitle)
?>
\$\endgroup\$
  • \$\begingroup\$ This is brilliant! Dang I didn't realised I'd mixed the get/set things up, Its a good convention to follow though. Another quick question the contstruct checks to see if there is a filepath set, how Do I now put a filepath for it to use in. \$\endgroup\$ – user2478101 Jul 12 '13 at 8:40
  • \$\begingroup\$ This is brilliant! Dang I didn't realised I'd mixed the get/set things up, Its a good convention to follow though. I'll study this code carefully, the next goal for me to do is to make a thumbnail class and output a fixed width with a dynamic height, Think this is possible with some functions in php and the floor() function. - I've seen where to add the filepath, just thought it would need to be added before the construct or does it not matter where you put it in a piece of code like that? thanks again \$\endgroup\$ – user2478101 Jul 12 '13 at 9:03
  • \$\begingroup\$ Some remarks: class Image would make someone believe it is an Image you are pointing to, but in fact. you are pointing to an ImageFolder. \$\endgroup\$ – Pinoniq Jul 12 '13 at 10:17
  • \$\begingroup\$ Taken on board thankyou for your input \$\endgroup\$ – user2478101 Jul 12 '13 at 13:42
  • \$\begingroup\$ Correct @Pinoniq - thanks for pointing that out! Class names should detail the class purpose / what the class works on or does. As for the construct and where the filepath goes - the class expects the filepath to be passed to it upon initialization, so basically when the class is instantiated via $image = new Image("/home/jsanc623/Pictures/"); - that path that is given is the filepath that is required. Also, yes dynamic height and fixed width is possible in PHP with either PHP GD or ImageMagick (with which you can do dynamic width or dynamic height by giving one or the other) \$\endgroup\$ – jsanc623 Jul 12 '13 at 13:57
0
\$\begingroup\$

The following pieces of code are almost identical in functionality:

$image = new Image( '/path/to/image.png' );
$path = $image->getImageFilepath();

$image = new Image( '/path/to/image.png' );
$path = $image->imageFilepath;

The first two lines employ encapsulation, but all four lines do not employ information hiding. Any class in the system can "get" the file path encapsulated by the Image class, which renders the lines functionally identical.

To develop software using object-oriented programming techniques, think about classes in terms of behaviours (i.e., methods), rather than attributes (i.e., data).

Why does any other code in the system need to know the image's file path? Chances are you can probably write instead:

$image = new Image( '/path/to/image.png' );
$result = $image->isPath( '/page/to/image.png' );

This allows you to also write:

$result = $image->isFilename( 'image.png' );

Most people erroneously resort to get method accessors without thinking about the underlying functionality. More often than not, slapping a get method into a class is an incorrect and limiting approach that will lead to inflexible, duplicate code. In other words, public accessor methods (get/set) are a sign of a functional design (similar to C structures) rather than an object-oriented design.

Pretend a method called "getFileExtension" exists that helps objects determine the type of image. All your objects that wanted to discern the type of image would have to call:

$extension = $image->getFileExtension();

Now what? All the objects would then have to check the file type:

if( $extension === "png" ) {
  // ... do something
}

This introduces a bug, duplicate code, magic variables (what is "png" and do you have to check for both "jpg" and "jpeg"; what about "jfif" extensions?), does not inform the reader why the extension is being retrieved, and is inflexible. Consider a more sophisticated solution that determines the image type using its "header" (the first few bytes in the file). This reveals the code that should have been written in the first place:

$result = $image->isImageType( $IMAGE_PNG );

Now all the clients must determine the file type as follows:

if( $image->isImageType( $IMAGE_PNG ) ) {
  // ... do something
}

It is trivial to see how the implementation could change from checking the file extension to checking the file contents without affecting any client. Not a single object that uses the Image class would have to change. If you wanted to support JPEG files, again, the clients need only ask:

if( $image->isImageType( $IMAGE_JPEG ) ) {
  // ... do something
}

No need to scatter knowledge about "jpeg", "jpg", and "JFIF" extensions throughout the codebase. Such knowledge should be encapsulated in a single location (DRY principle) and the file name hidden from other classes.

That's where the power of object-orientation resides.

Yet hiding how the file type is determined is still not fully object-oriented. To be object-oriented, the code for the Image class should expose behaviours that clients require. Clients don't really require knowledge about the image type or its path (especially since the path is required to instantiate an Image). Chances are the clients would want to perform some kind of manipulation on the image. What are those manipulations? Some examples:

  • Scale, shear, rotate
  • Convert to black and white
  • Convert file format
  • Create a thumbnail

Those are proper behaviours. A fully object-oriented Image class would be used as follows:

$image = new Image( '/path/to/file.png' );
$image->scale( 0.5 );
$image->rotate( 30 );
$image->index( 2 );
$image->thumbnail( '/path/to/thumbnail/file.jpg', 200, 200 ); // max width, height
$image->save( '/path/to/file.jpg' );
\$\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.