3
\$\begingroup\$

I have written a simple image cache script, the purpose is to load the image locally, so i can control the cache times of the images, and i have better load times than the host server, (battle.net serves 350 ms, i serve 65ish ms) this times 20 images is a pretty good performance step.

But being the performance freak i am. I was wondering if i can make it run faster!

Please also include the following in the review:

  • Better suggestions for function/variable names.
  • Better writing of code.
  • Alternate formatting.
  • Something that could be changed for something better / more correct.
  • Suggestions about how to make this generic / reusable.
  • An explanation about why you are right.
  • An explanation why you down vote this post.

So we all can learn from this.

Comments are only for Code Review and not actually in the file.

<?php
// Checks if the img parameter is set. 
if(!isset($_GET['img'])){
    die;
}
// Saves it as a variable
$url = $_GET['img'];
// Compiles the local file string
$img = dirname(__FILE__) . '/media/cache/' . get_image_from_url($url);

// Check if the file exists, if not it dowloads a version of the image.
if(!file_exists($img)){
    // log_message('info', 'file does not exists');
   download_image($img, $url);
}

// Checks if the image existing is more than 24 hours old if it is gets a new image.
if(time()-filemtime($img) > 24 * 3600){
  //  log_message('info', 'file is too old');
    download_image($img, $url);
}

header('Content-Type: image/jpeg');
$handle = imagecreatefromjpeg($img);
imagejpeg($handle);

/*
 * FUNCTIONS
 */

function get_image_from_url($url){
    $url = str_replace('http://eu.battle.net/static-render/','', $url);
    $url = str_replace('/','-',$url);
    return $url;   
}

function download_image($img, $url){
    $headers = get_headers($url);
    $response_code = substr($headers[0], 9, 3);

    if($response_code == '404'){

        // Pleas also advice here to make the file more generic.
        $url = 'http://localhost/warlords' . '/media/images/anonymous.jpg';
    }

    $thisurl = file_get_contents($url);    
    file_put_contents($img, $thisurl);   
}
\$\endgroup\$
5
\$\begingroup\$

When I want to cache some file, I'm typically doing it similar to your solution.

However, you're able to do most of this with built-in functionality, making the code a lot shorter:

<?php
// Some URL for testing
$url = 'https://www.google.de/favicon.ico';

// Time to cache the files (here: 10 minutes)
define('time_to_cache', 600);

// Create a local file representation
$local = './cache/' . urlencode($url);

// Determine whether the local file is too old
if (@filemtime($local) + time_to_cache < time()) {
    // Download a fresh copy
    copy ($url, $local);

    // Store headers in case we need them (see alternative below)
    file_put_contents($local . '.hdr', join($http_response_header, "\n"));
}

// Solution 1: Redirect to the local cache file
header('Location: ' . urlencode($local));
exit();

// Alternative: Send headers and the actual file
// Note that this might cause problems, e.g. due
// to cache fields and the like.

// Read and send headers
foreach(file($local . '.hdr') as $line)
    header($line);

// Read and send the actual file
readfile($local);

This doesn't include any error handling though.

For error handling, you might just want to check the results of the copy() operation. There's no need to request headers in advance or doing anything like that, since that will essentially delay you twice.

Regarding your implementation above:

  • Don't check the headers first. Just try downloading the file. This will take the same time. If you request headers first, you'll do two request, each of which might delay you by the mentioned 200-300 ms.

  • I've used urlencode() to create a working local filename. This might not be perfect depending on your actual file system though.

  • Don't check whether your local file exists first; you don't have to. You can just prepend filemtime() by an @, which will keep it from outputting any error messages and still return 0 in case the file doesn't exist (which will simply be interpretes as the file being ancient).

  • Why do you read the JPEG from your local disk and then reencoding it as a new image to be sent to the browser? Just send the actual file contents (or redirect the browser). The following will do the same without reencoding the image:

    header('Content-type: image/jpeg');
    readfile($local);
    
\$\endgroup\$
  • \$\begingroup\$ The reason i check the header is that if there isn't an existing image, i have a placeholder image i place instead. get_image_from_url makes a filename from the URL. ill check with the @filemtime when i get a chance. And i don't know why i read the JPEG file and reencoding it, thats why i asked it to be reviewed. Got a suggestion to improve it? \$\endgroup\$ – Magic-Mouse Dec 8 '14 at 13:42
  • \$\begingroup\$ You can just send the file contents using readfile() (see my last line of code). So you'd just write your header the way you do, then use readfile() rather than opening/generating the JPEG stuff. \$\endgroup\$ – Mario Dec 8 '14 at 13:43
  • \$\begingroup\$ this is great, it went from 65 ms to 35 ms. This reduced the load time on a page with 20 characters with about half a second. \$\endgroup\$ – Magic-Mouse Dec 8 '14 at 14:38
  • \$\begingroup\$ IMO “Don't check whether your local file exists first” is a bad advise. @-signs only hide errors and make code harder to debug. \$\endgroup\$ – feeela Dec 8 '14 at 16:31
  • \$\begingroup\$ @feeela IMO heavily depends on the actual code. In this example you could just add some comment // if file is too old or doesn't exist (timestamp is 0). \$\endgroup\$ – Mario Dec 8 '14 at 16:38

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.