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.

I am relatively new to JavaScript/jQuery, and cobbled together a script. Essentially, the script sets individual timers on specific DOM elements, and then recursively applies timed functions to them. The functions actually apply a "flip" effect to a div, showing a front and back, and then do different things on each side. The references to "Vague" are from Vague.js, and external library I didn't write that provides a blur effect to elements. The script works, but writing working code and writing good code are two separate things.

$(document).ready( function() {
    $('.flipper').each(function(e) {
        var flipper = new myFlipper($(this));
    })
});

var myFlipper = function (flipper) {
    //console.log(flipper);
    this.flipper = flipper;
    this.image = flipper.find('img');
    this.blurStatus = true;
    this.flipStatus = true;
    this.Vague = this.image.Vague ({
        intensity: 8,
        forceSVGUrl: false
    });
    this.blurContext = $.proxy(this.myBlur, this);
    this.flipContext = $.proxy(this.myFlip, this);
    // $(this.image).on("click", this.blurContext);
    var randomNumber = Math.floor(Math.random()*10);
    var timer = randomNumber * 1000;
    setTimeout(this.blurContext, timer);
}

myFlipper.prototype.myBlur = function(e) {
    console.log(this.flipContext);
    if(this.blurStatus) {
        this.Vague.blur();
        this.blurStatus = false;
        this.flipper.find('.caption').animate({opacity: 1.0}, 3000);
        setTimeout(this.flipContext,5000);
    } else {
        this.Vague.unblur();
        this.blurStatus = true;
    }
}

myFlipper.prototype.myFlip = function(e) {
    console.log(this.flipper.attr("class"));
    console.log(this.flipStatus);
    if (this.flipStatus) {
        this.flipper.addClass('hover');
        this.flipper.find('.caption').animate({opacity: 0.0}, 1000);
        this.blurContext();
        this.flipStatus = false;
        setTimeout(this.flipContext, 20000);
    } else {
        this.flipper.removeClass('hover');
        this.flipStatus = true;
        setTimeout(this.blurContext,5000);
    }
}

Here is the JSFiddle of the whole thing.

share|improve this question

2 Answers 2

up vote 8 down vote accepted

This might not be an improvement per se, but you can change this:

$(document).ready( function() {
    $('.flipper').each(function(e) {
        var flipper = new myFlipper($(this));
    })
});

To this:

$(function() {
    $('.flipper').each(function(e) {
        var flipper = new myFlipper($(this));
    });
});

The reason this works is $() is a shortcut for $(document).ready().

With JavaScript, it is usually recommended that you keep from cluttering the global namespace. You currently have everything contained in var myFlipper, which is good. But since you're using jQuery, you might as well use a jQuery plugin, which would allow you to do something like this:

$.fn.myFlipper = function () {
    //...
};

$(function() {
    $('.flipper').myFlipper();
});

This way, you aren't introducing another variable into the global namespace. Plus, the jQuery plugin architecture affords you some additional benefits, like syntax, etc.

share|improve this answer
    
On the second example:with $('.flipper').myFlipper();, I would NOT need to pass $(this) like I do to my current function because it's passing it to the function automatically, correct? Would I even need the 'each' function at that point, or would the plugin interact on each .flipper element as separately? –  xianritchie Mar 5 at 17:29
    
Additionally, using the plugin route, are my prototype calls still valid? Do I need to address myFlipper in the $.fn namespace when I refer to it, like: $.fn.myFlipper.prototype... or would the current syntax still be valid? –  xianritchie Mar 5 at 17:33
    
To your first question: correct, you would not need to pass $(this) to the jQuery plugin. The each function would be in the plugin code. To your second question: I would assume you can use prototype inside a jQuery plugin, but I've never tried it. Read this, post your revisions, and we can critique some more. –  Koveras Mar 5 at 18:06

From a once over,

  • You are not doing anything with flipper in var flipper = new myFlipper($(this)); you might as well just new myFlipper($(this));
  • Of course, that shows that myFlipper is not the best of names, perhaps simply Flip?
  • If you are not going to use e, then you do not need to declare it as a variable
  • As @DavidKennedy mentioned, you can simply use

    $(function() {
        $('.flipper').each(function() {
            new myFlipper($(this)); //Or startFlipping( $(this) );
        });
    });
    
  • Please indent inside your functions for readability
  • Production code does not use console.log
  • Production code does not have commented out code
  • You can show the flipping of the boolean blurStatus better like this:

    if(this.blurStatus) {
      this.Vague.blur();
      this.flipper.find('.caption').animate({opacity: 1.0}, 3000);
      setTimeout(this.flipContext,5000);
    } else {
      this.Vague.unblur();
    }
    this.blurStatus = !this.blurStatus; //<- Flipping happens here
    
  • The following code seems too long to me:

    var randomNumber = ;
    var timer = ;
    setTimeout(this.blurContext, timer);
    

    I would write it likes this, you should find a middle ground.

    setTimeout(this.blurContext, Math.floor(Math.random()*10000 );
    

    Since there is not much difference between say 8 seconds or 8.345 seconds, I just used Math.random()*10000

  • Consider using $.toggleClass('hover') instead of setting or removing it each time in if blocks.
  • You should use for strings either all single or all double quotes, the preference du jour is single quotes
  • You have some constants like 5000 that you repeat, consider using named constants instead
share|improve this answer
    
Thanks for this, too. I'm going to work on incorporating these changes and repost my new code. As for the indentation, I think it's an artifact from pasting code here. It's indented in Sublime Text, but not when I paste it. I don't know how to add 4 spaces to every line I paste except manually, so I just indent the flush lines. until the snippet all lists as code. –  xianritchie Mar 5 at 21:02

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.