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 have a small piece of code, and the problem is that I don't know how to organize it. There are tons of tutorials about JS code organization but I feel that those are for large scale apps. Also, I'm not sure if having the setInterval function where it is now is a big no no, can you come up with some critique/ideas?

YUI().use('node', function (Y) {
    // Access DOM nodes.

    var chapterTime = [0, 30, 49, 68, 90, 105, 127, 153, 182, 209],
        buttons = Y.one('ul'),
        currentChapter = 0,
        prevChapter = 0,
        myPlayer = videojs('example_video_1');

    buttons.delegate('click', function () {
        var button_name =  this.ancestor().get('id'),
            chapter = button_name.slice(1);

            playChapter(chapterTime[chapter-1], this);
    }, 'a');    

    function playChapter(chTime, chButton) {        
        myPlayer.play();
        setTimeout(function(){ myPlayer.currentTime(chTime); },100);
        Y.all('#nav a').setStyle('backgroundColor', "#999");
        chButton.setStyle('backgroundColor', '#666');
    }


    setInterval(function(){
        var playbackTime = myPlayer.currentTime(),
            i = 0,
            chapterFound = false;
            //console.log(chapterTime);


        while (i <= 9 && chapterFound == false){
            if (playbackTime >= chapterTime[i] && playbackTime < chapterTime[i+1]){
                currentChapter = i + 1;
                chapterFound = true;
            } else {
                i++;
            }
        } 

        if (currentChapter != prevChapter){
            var button = Y.one('#c' + currentChapter);
            Y.all('#nav a').setStyle('backgroundColor', '#999');
            button.one('a').setStyle('backgroundColor', '#666');
            prevChapter = currentChapter;
        }
    },1000);
});
share|improve this question

1 Answer 1

This looks just fine to me.

Some very minor nitpicks:

  • Remove //console.log(chapterTime);
  • Consider removing the blank line after var statements, it does not add much
  • chapterTime could use a comment as to what those numbers express (I assume seconds)
  • I would probably write this

    buttons.delegate('click', function () {
        var button_name =  this.ancestor().get('id'),
            chapter = button_name.slice(1);
    
            playChapter(chapterTime[chapter-1], this);
    }, 'a');   
    

    as

    buttons.delegate('click', function () {
            var chapter = this.ancestor().get('id').slice(1) - 1;
            playChapter(chapterTime[chapter], this);
    }, 'a');   
    
  • The color strings #999 and #666 have a meaning, I assume enabled/disabled, you could use properly named constants for these, declared on top
  • Only seldom use double newlines, they are vertical overkill

Still, all in all I like your code.

share|improve this answer

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.