Check if the jQuery library is present locally

Question

Q.

Basically this piece of code appears in my class constructor. It's working, but it seems very confusing.

It basically checks if the jQuery library is present locally and if it's not it will try to download it.

Is there any way I can improve this? Namely avoiding re-setting vars all the time?

---

CODE:

//First we check if JQuery version is correct 
if ( isset($versionList->$version) ) {
    $filePathFull = __DIR__ . '/versions/jquery-' . $version . '.js';
    $filePathMin  = __DIR__ . '/versions/jquery-' . $version . '.min.js';
    $this->versionLoaded = $version;

    //Now we check if the JQuery version is installed
    if ( !file_exists($filePathFull) || !file_exists($filePathMin)) {

        // They are not installed, so we will try to download and install it
        if (!self::installVersion($version)) {
            // Download failed, so we try the defaults
            $filePathFull = __DIR__ . '/versions/jquery-' . JQuery::DEFAULT_VERSION . '.js';
            $filePathMin = __DIR__ . '/versions/jquery-' . JQuery::DEFAULT_VERSION . '.min.js';
            $this->versionLoaded = JQuery::DEFAULT_VERSION;
        }
    }
} else {
    //Version is incorrect we go with the defaults
    $filePathFull = __DIR__ . '/versions/jquery-' . JQuery::DEFAULT_VERSION . '.js';
    $filePathMin = __DIR__ . '/versions/jquery-' . JQuery::DEFAULT_VERSION . '.min.js';
    $this->versionLoaded = JQuery::DEFAULT_VERSION;
}

//This is a redundant check to see if the defaults are installed
if ( !file_exists($filePathFull) || !file_exists($filePathMin) ) {

    // They are not installed, so we will try to download and install it
    if (!self::installVersion(JQuery::DEFAULT_VERSION)) {
        //Defaults are not installed so we throw an exception
        throw new Exception("Nor the version $version or the default version " . JQuery::DEFAULT_VERSION . " could be found or downloaded");
    }
}

---

Edit:

MORE DETAILS

A more detailed explanation...

As you correctly assumed, this is a "plugin". The plugable system requires wrapped JavaScript libraries in PHP classes that are used for a JS Dependency Manager. The JS Dependency Manager extends Composer which constrains how we handle this a bit. We have, however, freedom to chose how we wrap the JS Library.

Goals:

1. Avoidance of library repetition (like having 2 versions of jQuery running which causes weird JS bugs)
2. Automatic update of 3rd party JavaScript (directly from the repository instead of downloading and including them directly)
3. Caching, Minimizing and removal of dead code from JS Scripts through Google Closure Compiler
4. Libraries and scripts dependency management (specially regarding specific library versions such as jquery)

Answer 1

Well, you could set this up to use additional methods. You don't have to throw everything into the constructor. This would be a big step in the right direction, at least in as far as OOP and best practice is concerned. This will help you to understand what is going on much easier while also helping you to avoid violating basic OOP principles. For instance:

Single Responsibility

The first principle you are violating, or so it seems, is the Single Responsibility Principle. As the name implies, your classes and methods should be responsible for just one thing, to the exclusion of all others. They can request information from others if necessary, but they should not be aware of how those others work. This principle is typically demonstrated with a function/method, but should also be applied to the classes themselves.

It's hard to tell in this context, because all you provided were the guts, but, judging by the static properties, I would assume you are handling these loading failures for another class. If you are not able to modify the class in question, such as if this were a library, then this functionality should be added to an intermediary, such as a controller, otherwise this really belongs in the JQuery class. Let's continue under the assumption that this is being constructed in said intermediary.

Don't Repeat Yourself

The "Don't Repeat Yourself" (DRY) Principle is another of the principles you are violating. As the name implies, your code should not repeat. Typically this is done through functions/methods, but it can also be done with variables/properties and loops, though it is not just confined to this. Keeping your code DRY takes a lot of foresight, but really helps in the long run. So let's take a look at how your code is repeating itself and try to fix this.

This should be fairly obvious. There are two files that are being verifyed and the same things are being done to both files. To paraphrase someone important to DRY (can't remember who, only that it was similar to the Rule of Three): we can either do something once, or do something many times; There is no such thing as doing something only twice. To follow this pearl of wisdom, we can create a method, and then use that method each time we need it. This also follows the Single Responsibility principle because it abstracts the task away from the constructor, whose only job is to instantiate initial values.

public function verifyVersion( $file, $version ) {
    //generalized verification code
}

As the comment says, this method should be generalized for a single file, which can either have a boolean return value or a boolean property it manipulates to show whether verification was a success. It seems like you already have such a property in $versionLoaded so we can use that. Its not exactly a boolean, but it can be used as one so long as its default value is FALSE or NULL.

if( ! $this->versionLoaded ) {
    throw new Exception(
        "Neither the version $version nor the default version "
        . JQuery::DEFAULT_VERSION
        . " could be found or downloaded"
    );
}

BTW: I slightly edited your exception because the grammar was bothering me. "Nor" is the negative connotation of "or" and "neither" prefixes a list of negatives. Though I'm not an English major, so you can take that with a grain of salt.

How do we generalize our check? And equally important, how do we do it just once? In your code you were checking if these files exist not once, but twice. If we check it once, and assume that if the version is loaded then the file exists, then we wont need to check it a second time.

if( file_exists( $file ) || self::installVersion( $version ) ) {
    $this->versionLoaded = $version;
}

That should leave us with one final task. How to check each file. Well, I mentioned that loops were sometimes used in DRY. Here's a pretty good time to use one.

$header  = __DIR__ . '/versions/jquery-';
$default = JQuery::DEFAULT_VERSION;

$files = array(
    $header . $version . '.js',
    $header . $version . '.min.js',
    $header . $default . '.js',
    $header . $default . '.min.js'
);

foreach( $files AS $file ) {
    $this->verifyVersion( $file, $version );
    if( $this->versionLoaded ) {
        return;
    }
}

throw new Exception(
    "Neither the version $version nor the default version "
    . JQuery::DEFAULT_VERSION
    . " could be found or downloaded"
);

Of course, this is still violating the Single Responsibility Principle. This constructor is still doing too much, but I'll leave that up to you to refactor. If you use everything I posted above it should be pretty simple. I'll give you a couple of hints though:

• Those files are not violating DRY. You might be thinking that using $header and '.js' repeatedly is a violation, but that is to ensure that our method is as reusable as possible. The only way they might be is by their similar structure, which I can't see refactoring without confusing the issue. Though you might want to try that for yourself.
• I think a pretty good name for this new method might be loadJQuery().

I hope this helps!

Edit

I forgot about the isset( $versionList->$version ); bit, so the above code will need to be refactored for that as well, but I think the solution should be obvious once that last method is implemented. If you need clarification, let me know.

Answer 2

Try this, just removed a couple of the repeated code (moved it to a function) and sets minified version or regular version based on a bool variable (getminified), thus you only have filePathFull (no longer have filePathMin):

# Set `true` if minified version is preferred.
// let's assume its in a class somewhere
public $getMinified = true;

# DRY
function getFilePath($version){
    $start = __DIR__ . '/versions/jquery-';
    $end = ($someClass->getMinified == true ? '.min.js' : '.js');
    return $start . $version . $end;
}

# First we check if JQuery version is correct 
if ( isset($versionList->$version) ) {
    $filePathFull = getFilePath($version); // DRY
    $this->versionLoaded = $version;

    # Now we check if the JQuery version is installed
    // Why two if statements? Lets just concat the ifs 
    if ( !file_exists($filePathFull) && !self::installVersion($version) ) {
        # Download failed, so we try the defaults
        $filePathFull = getFilePath(JQuery::DEFAULT_VERSION); // DRY
        $this->versionLoaded = JQuery::DEFAULT_VERSION;
    }
} else {
    #Version is incorrect we go with the defaults
    $filePathFull = getFilePath(JQuery::DEFAULT_VERSION); // DRY
    $this->versionLoaded = JQuery::DEFAULT_VERSION;
}


#This is a redundant check to see if the defaults are installed
if ( !file_exists($filePathFull) ) {

    # They are not installed, so we will try to download and install it
    if (!self::installVersion(JQuery::DEFAULT_VERSION)) {
        #Defaults are not installed so we throw an exception
        throw new Exception("Nor the version $version or the default version " . JQuery::DEFAULT_VERSION . " could be found or downloaded");
    }
}