SitePoint Sponsor

User Tag List

Results 1 to 3 of 3

Hybrid View

  1. #1
    Spirit Coder allspiritseve's Avatar
    Join Date
    Dec 2002
    Location
    Ann Arbor, MI (USA)
    Posts
    648
    Mentioned
    0 Post(s)
    Tagged
    0 Thread(s)

    Refactoring image handling in controller

    I made a little class to handle images, resizing and stuff. I also made a little factory that takes an image in $_FILES and turns it into an Image object. I also made a little gateway that handles inserting a record into the db for each iamge. And yet... I don't even have error handling yet and my code looks like this:

    PHP Code:
            if (strlen ($_FILES['image']['name']) > 0)    {
                
    $image_data $this->imagesGateway->insert(array ('filename' => ''));
                
    $image_data['filename'] = 'images/image' $image_data['id'] . '.jpg';
                
    $this->imagesGateway->update ($image_data['id'], $image_data);
                
    $image $this->factory->create ($_FILES['image']);
                
    $image->saveJpeg ('../' $image_data['filename']);
                
    $_POST['image_id'] = $image_data['id'];
            } elseif (
    $_POST['remove_image'])    {
                
    $this->imagesGateway->delete ($page->image_id);
                @
    unlink ('../images/image' $page->image_id '.jpg');
            }
            
    $this->pagesGateway->update ($page->id$_POST); 
    I have never seen a clean way of handling images right from the request... most examples I see are using PHP functions and have no abstraction whatsoever.

    I feel like this is way too much for the controller to handle. I would love it if I could refactor out some code into something like this:

    PHP Code:
            if ($this->files->get ('image'))    {
                try {
                    
    $image $this->imagesGateway->save ($this->files->get ('image'));
                } catch (
    Exception $e)    {
                    
    $this->addError ($e->getMessage());
                }
                
    $page->add ($image);
                
    $pagesGatway->update ($page->id$page)
            } 
    Aside from just copying the code above, I have a lot of design decisions to make. Should I have an intercepting filter that watches for images in $_FILES and makes Image objects available in the controller? If so, should the intercepting filter be looking for errors and providing them to the controller as well? If there was an upload error I can't create an image handle from the image, so it would have to have at least a small amount of error handling.

    And then, I've got two separate places I need to save that image: physically in the filesystem, preferably with a name that links it to a record in the database ('images/image60.jpg') and then I need to save in the database, preferably with the whole url. I could make two separate gateways, but as you can see these overlap. I could make one gateway... but I worry that I won't have enough control at that point. What if I need to save an image in another location? What if I need images saved in a different part of the database?

    In short, I feel like there are things I can be doing to make this controller much lighter, but I'm not really sure what those things are...

  2. #2
    SitePoint Guru dagfinn's Avatar
    Join Date
    Jan 2004
    Location
    Oslo, Norway
    Posts
    894
    Mentioned
    0 Post(s)
    Tagged
    0 Thread(s)
    I think you have the right general idea about how you want to refactor this code. I'm tempted to say "just do it". It has to be done in small steps, and you're sure to discover something new along the way.

    I don't completely follow your ideas about one or two gateways, but it "feels" like the kind of situation where you might want two and then another class to coordinate them.
    Dagfinn Reiersøl
    PHP in Action / Blog / Twitter
    "Making the impossible possible, the possible easy,
    and the easy elegant"
    -- Moshe Feldenkrais

  3. #3
    Spirit Coder allspiritseve's Avatar
    Join Date
    Dec 2002
    Location
    Ann Arbor, MI (USA)
    Posts
    648
    Mentioned
    0 Post(s)
    Tagged
    0 Thread(s)
    Quote Originally Posted by dagfinn View Post
    I think you have the right general idea about how you want to refactor this code. I'm tempted to say "just do it". It has to be done in small steps, and you're sure to discover something new along the way.
    Will do.

    Quote Originally Posted by dagfinn View Post
    I don't completely follow your ideas about one or two gateways, but it "feels" like the kind of situation where you might want two and then another class to coordinate them.
    Well, just because gateways are supposed to be tied to a specific source, so it seems like one for working with the filesystem and one for working with the db would be appropriate. Since they're so closely tied though, maybe your coordinating class is a good idea.

    One thing I have been thinking about that might make things simpler off the bat is to create an Image object with the temporary filename and information, save it in the db, and then just update the db when I move it. That seems much cleaner than saving an empty filename just to get an id.


Tags for this Thread

Bookmarks

Posting Permissions

  • You may not post new threads
  • You may not post replies
  • You may not post attachments
  • You may not edit your posts
  •