Check if the jQuery library is present locally

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, thi

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 issu

StackExchange Code Review Q#17803, answer score: 3