Take the 2-minute tour ×
Code Review Stack Exchange is a question and answer site for peer programmer code reviews. It's 100% free, no registration required.

First question on this site so please take it easy on me and let me know if I infringe the common etiquette :-)

I have written the following script which simply:

  1. Loops through javascript files (either defined in a particular package, or separately with the libraries array)
  2. Reads the files into the script and combines them into one string
  3. Minifies the string and saves the contents to a cached file on the server and returns the contents.

Some key features:

  1. The cached file is only updated when the script detects that one of the loaded javascript libraries has been modified
  2. The script only gathers the files if a cached version does not exist

UPDATE 1

One thing I forgot to mention as I am sure most of us programmers will notice; the array of libraries could very easily be built dynamically by scanning the _js/_source/ directory and therefore I would not have to add each library to the $js_libraries array individually. The reason I have not done this is simple, it is much slower when there are a lot of files and the absolute key of this script is speed!

UPDATE 2

I forgot to actually ask my question! I simply want to know whether this could be written better and whether it has any areas I could improve on.

Here is my code:

<?php

/**
 * Sine Macula Package API
 * The Sine Macula Package API loads and packs documents ready to
 * be returned to the browser
 * @name jsapi.php
 * @author Ben Carey
 * @version 1.0
 * @date 24/10/2012
 * @copyright (c) 2012 Sine Macula Limited (sinemaculammviii.com)
 */

// Retrieve the supplied variables
$package    = $_REQUEST['package'] ? strtolower($_REQUEST['package']) : 'all';
$packageURL = $_REQUEST['packageURL'];
$libraries  = strtolower($_REQUEST['libraries']);

// Define all the javascript libraries
$js_libraries = array(
    'md5'           => '/_js/_source/_md5/sinemacula.md5.js',
    'cookies'       => '/_js/_source/_cookies/sinemacula.cookies.js',
    'preloading'    => '/_js/_source/_preloading/sinemacula.preloading.js',
    'browsers'      => '/_js/_source/_browsers/sinemacula.browsers.js',
    'overlay'       => '/_js/_source/_overlay/sinemacula.overlay.js',
    'forms'         => '/_js/_source/_forms/sinemacula.forms.js',
    'fancybox'      => '/_js/_source/_fancybox/jQuery.lightbox-0.5.js',
    'indexof'       => '/_js/_source/_indexOf/init.indexOf.js'
    // This array will be much larger
);

// Only define the javascript packages if the
// package is not coming from an external source
if($package){
    // Define all the javascript packages
    $js_packages = array(
        'all' => array_keys($js_libraries)
        // This array will be much larger 
    );
    // Make sure the supplied package exists
    if(array_key_exists($package,$js_packages)){
        // Gather all the libraries for the particular package
        $package = array_intersect_key($js_libraries,array_flip($js_packages[$package]));
    }else{
        $errors[] = 'The supplied package does not exist';  
    }
}elseif($packageURL){
    // Attempt to retrieve the contents of the external package
    $package = @file_get_contents($packageURL);
    if($package){
        // Decode the JSON object into an array
        $package = json_decode($package,true);
    }else{
        $errors[] = 'Error retrieving package from remote source';  
    }
}

// Add the libraries to the package array
// if they have been supplied
if($libraries){
    // Explode the libraries string into an array
    $libraries = explode(',',$libraries);
    // Gather the unrecognised libraries
    $unrecognised_libraries = array_diff($libraries,array_keys($js_libraries));
    // If there are unrecognised libraries then
    // return relevant errors
    if($unrecognised_libraries){
        foreach($unrecognised_libraries as $value){
            $errors[] = '\''.$value.'\' library does not exist';
        }
    }
    // Insert all the sources for the library keys
    $libraries = array_intersect_key($js_libraries,array_flip($libraries));
    // Make sure package is an array
    $package = is_array($package) ? $package : array();
    // Merge the libraries with the package array
    $package = array_merge($package,$libraries);
}

// Create a hash of the package
$package_hash = md5(implode('',array_keys($package)));

// Loop through the package and check their modified dates
$modified_timestamps = array_map(function($a){
    // Get the modified time of each individual library
    return filemtime(dirname(__FILE__).$a);
},$package);
// Hash the modified time, this will determine whether the,
// library has been updated
$modified_hash = md5(implode('',$modified_timestamps));

// Check to see if the package directory exists in the cache
if(@is_dir(dirname(__FILE__).'/_js/_cache/_' . $package_hash )){
    // If the modified hash matches a file in the folder then it
    // does not need updating
    if(file_exists(dirname(__FILE__).'/_js/_cache/_'.$package_hash.'/'.$modified_hash.'.js')){
        // Get the contents of the file
        $packed_script = file_get_contents(dirname(__FILE__).'/_js/_cache/_'.$package_hash.'/'.$modified_hash.'.js');
    }else{
        // Set pack to true to ensure that the script
        // is created
        $pack = true;   
    }
}else{
    // Create the directory for the package
    @mkdir(dirname(__FILE__).'/_js/_cache/_'.$package_hash);
    // Set pack to true to ensure that the script
    // is created
    $pack = true;
}

// If pack is set then gather all the files, combine
// into one and pack. This will create the file as
// well, and store it in the folder '_cache'
if($pack){
    // Loop through the package and load all of the libraries
    // into the script ready to be packed
    foreach($package as $key => $value){
        // Get the script
        $current_script = @file_get_contents(dirname(__FILE__).$value);
        // Return an error on fail to load
        if($current_script){
            $combined_script .= $current_script.";\n";  
        }else{
            $errors[] = 'Failed to load \''.$key.'\' library';  
        }
    }   
    // Include the javascript packer
    require_once(dirname(__FILE__).'/_php/_classes/class.packer.php');
    // Initiate the packer and pack the javascript
    $js_packer = new JavaScriptPacker($combined_script);
    $packed_script = $js_packer->pack();
    // Retrieve the cached scripts (should only be one)
    $cached_scripts = @glob(dirname(__FILE__).'/_js/_cache/_'.$package_hash.'/*.js');
    // Delete all the files in the package directory
    if(is_array($cached_scripts)){
        array_map('unlink',$cached_scripts);
    }
    // Create the new javascript package file if there
    // have been no errors
    if(empty($errors)){
        if(!@file_put_contents(dirname(__FILE__).'/_js/_cache/_'.$package_hash.'/'.$modified_hash.'.js',$packed_script)){
            $errors[] = 'There was an error loading the javascript libraries';
        }
    }
}

// Set the content header to javascript
// and write the packed script or errors
header("Content-Type: text/javascript");
if(empty($errors)){
    echo $packed_script;
}else{
    // Loop through the errors and write them to the
    // javascript console
    foreach($errors as $value){
        echo 'console.log("'.$value.'");';
    }   
}

?>
share|improve this question

1 Answer 1

up vote 2 down vote accepted

Don't point at php

This script looks like the intention is to point at a php file, that returns the compressed content. Assuming that's the case...

PHP doesn't belong in-between a user and a static, public file. Webservers are much better for serving static content. Rather than pointing at a php script that processes the request, looks for and/or manipulates files and then serves it - take advantage of the webserver and point at the cache file directly - or where the cache file should be if it doesn't exist.

I.e. change this logic:

user -> /someurl.php?args -> php -> build/find static file -> serve it

To the equivalent of:

user -> /js/packet/hash.js

Putting as little as possible in-between the user and the content they are requesting means better performance.

Handle requests for missing packets

If you want to automatically handle requests for missing packets, add a .htaccess file (or equivalent) like this to the css/js folder:

<IfModule mod_rewrite.c>
    RewriteEngine On
    RewriteCond %{REQUEST_FILENAME} !-f
    RewriteRule ^(.*)$ handlemissingasset.php [L]
</IfModule>

You'll have the possibility to handle requests for a packed js/css file on the first request where it's missing. Write to the equivalent path of the current url - and the webserver will take care of serving the file and sending appropriate headers.

Derive the url at run time, not the file's contents

Instead of using a url which contains, serialized in some form, the contents that are to be returned - just derive the url for the packet file. So e.g.:

function url($packets) {
    return md5(implode($packets) . APP_VERSION) . '.js';
}

echo sprintf('<script src="%s"></script>', url(array('foo', 'bar')));

The less logic there is at runtime - the faster the code performs.

Use a build process

Rather than handle requests for missing packets, a process that builds them all, once, could be used.

$ . buildPackets.php
Scanning for packets
... found ('foo', 'bar')
... bumping APP_VERSION
... writing js/packets/d41d8cd98f00b204e9800998ecf8427e.js

The benefit of working in this way is that the run-time logic is reduced to the bare minimum, there's no need at any point of a web request to check if files exist ect. This is also more or less a pre-requisite to be able to use a CDN.

Explicit is better than implicit

I'm not sure what the logic at the top of the code in the question is for, but (and this is from experience). if in a project there are requests for:

'foo', 'bar'
'bar', 'foo'
'foo', 'bar', 'other'
'zum', 'bar', 'other'

It becomes difficult to automatically serve optimal packets.

If each individual request results in 1 js file that is sub optimal, as the user receives one file per page, but the same content multiple times as they browser around the site.

As such it's likely better to define all js packets in a config file and only refer to the packet names rather than permit dynamic usage.

Appropriate Headers

Whether you heed or ignore the above, this point is the biggest failing with the code as presented. There are 2 ways to handle headers for asset files (css, js images), the serverver and/or the php script needs to implement correct headers for optimal performance.

Validation requests

A short expiry is sent in the headers, the user sends a request and a successful response is either a 200 or a 304 - make sure to be sending 304s where appropriate.

This means a user downloads the file once on the first request, and on each subsequent request for the same asset they receive an empty 304 not modified response. Not sending the user back the same file saves bandwidth and increases speed.

Long expiry

If the url changes when an asset changes - there's no need for validation requests and you can send in the headers that the response is valid for - e.g. a year. This means a user downloads the file on the first request - and never requests it again. Validation requests are a great improvement over no/incorrect cache headers. Long expiry is another significant improvement.

Finally: Look for existing solutions

You may find repos like this one useful, either the code, the docs, the api or all of the above. Try to avoid the mistakes that many have already made before you :).

share|improve this answer
    
Thank you very much for your answer and pointers! :-) I have a few questions about some of the things you have mentioned. Firstly, the hash.js file is not static as it is rebuilt whenever any of the files in that package have been modified, this is essential as if I make a change to any of the libraries, the changes must appear on the sites immediately. Therefore, I cannot see a way of doing this without pointing to a PHP file. This is also why I cannot derive the URL at runtime as the hash will be different if any of the libraries have been modified. (continuing) –  Ben Carey Nov 1 '12 at 13:38
    
(continued) What do you mean about the $js_libraries array not being optimal? I can understand keeping the definition of this array in another file but don't understand what is wrong with it. As far as appropriate headers go, I only used what I found on the web, would you mind posting the correct headers to use for returning a javascript page? I agree about the cache, however, how will it know whether the cache file has been modified? Especially if it is not actually looking at it, it is looking at the PHP file instead? :-) –  Ben Carey Nov 1 '12 at 13:44
    
you're still thinking that "hash" indicates only the packets of the file - just bump APP_VERSION (referring to the example code) whenever a file changes, and the url changes. The main problem with your approach and IMO the first thing to change - is that you're pointing at a php file. If you don't change that, this code will always be of limited use. Regarding headers - just use something like yslow, or google page speed and look at their references. –  AD7six Nov 1 '12 at 14:47
    
I am modifying the files the whole time so bumping a constant is not a great method in this case. This will also mean that I will have to run a script each time I modify the files in order to minify them. Many applications point to PHP files to obtain javascript, I agree it is not a great method but sometimes it is necessary if the javascript you are loading is environment dependant. Do you not agree? –  Ben Carey Nov 1 '12 at 14:55
    
no :). You're trying to optimize for development, instead of optimizing for production/the end user, it's the wrong focus. The use of a single constant was mostly for illustration - know that the cost of file_exists, filemtime, md5_file etc. are relatively significant, and to be avoided. There are lots of scripts out there that run processes when files change - in development just use the same kind of thing. –  AD7six Nov 1 '12 at 16:17

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.