Take the 2-minute tour ×
Programmers Stack Exchange is a question and answer site for professional programmers interested in conceptual questions about software development. It's 100% free, no registration required.

I'm still trying to wrap my head around OOP. All of the following examples work, of course, but is there one (or possibly another) that best exemplifies OOP concepts?

/**
 * For the following examples the Image class queries an image
 * and associated info from a database, which is passed on
 * instantiation via dependency injection.
 */

$image = new Image(new Db());

Example 1:

if ($image->setImageId($id, $size)) {
    header('content-type: ' . $image->content_type);
    header('content-length: ' . $image->length);
    echo $image->getBytes();
}

Example 2:

if ($image_info = $image->getImageInfoByIdAndSize($id, $size)) {
    header('content-type: ' . $image_info->content_type);
    header('content-length: ' . $image_info->length);
    echo $image->getImageBytesByIdAndSize($id, $size);
}

Example 3:

$image->setImageIdAndSize($id, $size);

if ($image_info = $image->getImageInfo()) {
    header('content-type: ' . $image_info->content_type);
    header('content-length: ' . $image_info->length);
    echo $image->getImageBytes();
}
share|improve this question
2  
This probably better asked on Programmers.StackExchange.com. CodeReview.StackExchange.com is for ... code reviews. –  Ross Patterson Mar 1 '13 at 22:54
add comment

migrated from codereview.stackexchange.com Mar 15 '13 at 7:13

This question came from our site for peer programmer code reviews.

3 Answers

The Image class has two responsibilities that should be separated:

  • Abstract database access
  • Encapsulate image information

Extract the database access into a data access object (DAO).

class ImageDao
{
    private $db;

    public function __construct(Db $db) {
        $this->db = $db;
    }

    public function findByIdAndSize($id, $size) {
        return new Image($db->exec(...));
    }
}

This frees up Image to do what it should: store information about an image and possibly provide methods to manipulate it.

share|improve this answer
add comment

Personally I prefer #1.

#2 almost seems like you are trying to use a singleton pattern, where you could have dozens of such calls in a row with different $id's each time on the same object. If that is your intent, you should probably go all the way and make the calls like Image::getImageBytes($id)

#2 and #3 are both odd because it seems like you are loading some kind of sidecar data. This would be fine if the object composition was such that this side car data may represent another query to the DB (like if there is a table in your db called 'ImageComments' and you wanted to use $image->getImageComments() to return to you an array of 'ImageComments') OR a shared resource that multiple Images might need (like say a parent gallery) but it seems unlikely that an image (especially with a selectable size) would have a stored 'image_info' and even less likely that it would share that with other images.

I might suggest using public methods (i.e. $image->length()) instead of public instance variables to improve any of them (but especially #1). This would allow for you to do simple things like delay actually loading data from the DB until you actually need to. And in the case of any sidecar data as discussed earlier, the methods could take care of doing whatever background work was needed to accomplish the task (e.g. public function getImageComments() { return $this->DB("SELECT * FROM 'ImageComments' WHERE image_id = {$this->_id}"); } )

share|improve this answer
    
could you please expand on "sidecar data"? –  Isius Mar 5 '13 at 1:43
    
I thought it wise to pull all data in a single db query. Are you suggesting that I query individually for each field that I need? –  Isius Mar 5 '13 at 1:47
    
You should get as much from the DB as you can in a single query (as long as it is reasonable that the information will be used). You should probably NOT save raw image data directly in the DB, instead make a base 'File' class that is implemented as a DB object for metadata about files (like path). Then the subclass 'Image' can implement methods specific to Images like constructing new crops based on requested information. –  Jared Kipe Mar 16 '13 at 19:55
    
I did explain 'sidecar data' but I can try to give more examples. I could define 'sidecar data' as any information related to the original DB Object, usually accessed through a JOIN, a 'SELECT .. WHERE parent_id = id', or possibly by loading data a URL or disk (path). –  Jared Kipe Mar 16 '13 at 19:58
add comment

For something like this, where the data is (likely) immutable once it is loaded into the object, you should really have some sort of ImageFactory that encapsulates the loading of images, that way the Image class can be immutable.

share|improve this answer
add comment

Your Answer

 
discard

By posting your answer, you agree to the privacy policy and terms of service.

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