5
\$\begingroup\$

I'm fairly new to jQuery and have decided to make a jQuery audio player for a website.

There are two versions of the way it selects the audio:

  1. Here
  2. Here

I removed some basic, not connected with the player functions. What do you think of it?

$(document).ready(function() {



  function secondsTimeSpanToHMS(s) {
         // this function is not mine
    var m = Math.floor(s / 60); //Get remaining minutes
    s -= m * 60;
    return (m < 10 ? '0' + m : m) + ":" + (s < 10 ? '0' + Math.floor(s) : Math.floor(s)); //zero padding on minutes and seconds
  }



  // INDEX PLAYER

  $('.play').click(function() {




    var $this = $(this);
    var $master = $('.master');
    var id = $this.attr('id').replace(/btn/, '');

    $(".play").not(this).not('.master').each(function() { 
      if ($(this).hasClass("active"))
        $(this).click();
    });

    $this.toggleClass('active');

    var audio = $('audio[id^="sound"]')[id - 1];


    if ($this.hasClass('active')) {

      $master.attr('id', $(this).attr('id'));


      $this.addClass('playing');

      audio.play();

      $('#songlength').text(secondsTimeSpanToHMS(audio.duration));



      if (!$this.hasClass("master")) {
        if (!$(".master").hasClass("playing")){
          $master.addClass("playing");
          $master.addClass("active");

      }




      audio.ontimeupdate = function() {  
        $('.progress').css("width", (audio.currentTime / audio.duration * 100 + '%'));

        $('#duration').text(secondsTimeSpanToHMS(audio.currentTime));
      }

    } else {    
      $this.removeClass('playing');  
      audio.pause();

        if (!$this.hasClass("master")) {  
          if ($(".master").hasClass("playing"))
            $master.removeClass("playing"); 
        }
    }



    $('#slider').bind('click', function(ev) {

      var $div = $(ev.target);
      var $display = $div.find('#progcont');
      var vW = $(window).width(); 
      var pW = $("#progcont").width();
      var offset = $div.offset();
      var x = ev.clientX - offset.left;
      var ProcRatio = x / pW;
      audio.currentTime = ProcRatio * audio.duration;

    });



  });


  //INDEX.HTML BIG PLAY BUTTON


  $(".master").click(function() {
    var $this = $(this);
    var ID = $this.attr('id');
    console.log(ID);

    if ($this.hasClass("playing")) {
      console.log("ZROBI PLAY");
      $(".play").not(this).each(function() {
        if ($(this).attr("id") == ID)
          $(this).click();
      });

    } else {
      console.log("ZROBI PAUSE");
      $(".play").not(this).each(function() {
        if ($(this).hasClass('active')) {
          $(this).click();
        }
      });
    }
  });


  //PLAY ON MUZYKA
  $('.play2').click(function() {



    var $this = $(this);
    var audio = $(this).find('audio').get(0);



    $(".play2").not(this).each(function() {
      if ($(this).hasClass("active"))
        $(this).click();
    });

    $this.toggleClass('active');
    $('#bottomPlayer').css('bottom', '0px');


    if ($this.hasClass('active')) { //LAUNCHES THE TRACK



      $this.delay(6).addClass('playing');


      audio.play();
      audio.currentTime = 0;
      $('#songlength').text(secondsTimeSpanToHMS(audio.duration));

      audio.ontimeupdate = function() {
        $('.progress').css("width", audio.currentTime / audio.duration * 100 + '%');

        $('#duration').text(secondsTimeSpanToHMS(audio.currentTime));
      }

    } else {
      $this.removeClass('playing');
      audio.pause();
      $('#bottomPlayer').css('bottom', '-50px');
    }




    $('#slider').bind('click', function(ev) {

      var $div = $(ev.target);
      var $display = $div.find('#progcont');
      var vW = $(window).width();
      var pW = $("#progcont").width();
      var offset = $div.offset();
      var x = ev.clientX - offset.left;
      var ProcRatio = x / pW;

      audio.currentTime = ProcRatio * audio.duration;

    });


  });


  $('#pauseAll').click(function() {
    $(".play2").each(function() {
      if ($(this).hasClass("active"))
        $(this).click();
    });
    $('#bottomPlayer').css('bottom', '-50px');
  });



});
\$\endgroup\$
4
\$\begingroup\$

There are some problems with style and consistency. The indentation is all over the place and there is too much whitespace. Quotes are inconsistent; choose single or double quotes and stick to one. Some one-line only statements have braces, and others don't, better to always add the braces. You are relying on type casting here:

var id = $this.attr('id').replace(/btn/, ''); // `id` is a string
...
$('audio[id^="sound"]')[id - 1]; // but you use it as a number

An id is unique in the page, so this selector is overspecified:

var $display = $div.find('#progcont'); // same as `$('#progcont')`

PascalCase is the convention for JavaScript constructors, so this is confusing:

var ProcRatio = x / pW;

I'd suggest not wrapping your objects in jQuery when it is not necessary, like in this case:

if ($(this).attr("id") == ID) // same as `if (this.id === ID)`

Or this case:

$master.attr('id', $(this).attr('id')); // same as `$master.attr('id', this.id)`

Don't use bind in jQuery because it is deprecated, use on instead:

$('#slider').bind('click', function(ev) {

I don't think this works as you think it does:

$this.delay(6).addClass('playing');

delay works if there is a queue already, but there isn't one.

Avoid changing CSS in JS whenever possible. For example, you are repeating this code twice:

$('#bottomPlayer').css('bottom', '-50px');

Instead, create a class and toggle it on and off to make it reusable.

\$\endgroup\$
  • \$\begingroup\$ Thanks for the reply. I have corrected everything you said, except the var ProcRatio = x / pW; because I don't understand what you mean there. \$\endgroup\$ – aln447 Dec 7 '15 at 16:48
  • 1
    \$\begingroup\$ What I meant is that the variable name starts with capital letter (PascalCase) and this convention is reserved for constructors in JavaScript, notice the syntax highlighting? The convention for this variable would be to use camelCase, so var procRatio. \$\endgroup\$ – elclanrs Dec 7 '15 at 16:54
  • \$\begingroup\$ Got it. Fixed now \$\endgroup\$ – aln447 Dec 7 '15 at 17:40
1
\$\begingroup\$

Your ternary looks funny, I'd personally declare a function to do the zero padding more readably:

function zeroPad(num){
    return num < 10 ? '0' + num : num
}

...

return zeroPad(m) + ":" + zeroPad(Math.floor(s))
\$\endgroup\$
  • \$\begingroup\$ Like I said the HMS is not my function so I didn't really touch it because...well...it worked :P \$\endgroup\$ – aln447 Dec 7 '15 at 16:49
  • \$\begingroup\$ @aln447 Ah, this is what I get for not reading comments haha. \$\endgroup\$ – SuperBiasedMan Dec 7 '15 at 16:51
0
\$\begingroup\$

Thoughts and ideas ...

Major ideas

  • Currently confusing because $('.play') includes the $(".master") button. Yet there are click handlers for both $('.play') and $(".master"). It would make more sense to make the master button a proxy for the currently active play button.
  • Association of elements by id is a bad idea - if possible, traverse the DOM to find audio element relative to its play button (as you do with the play2 buttons but not the play buttons).
  • $master.attr('id', $(this).attr('id')) is illegal. No two elements should have the same id. May well cause a problem. (However not relevant if proxy idea is adopted).
  • When attaching event handlers inside event handlers - ensure there's no accumulation of effect by calling .off() before .on(), eg $("#slider").off('click').on('click', ...);
  • .delay(6) looks like a dangerous workaround. Is the delay correct in all browsers on all computers all of the time?
  • The ontimeupdate handlers are never detached, suggesting that they can be attached once, at the outset, to all audio elements. Properly phrased, a single handler should suffice.
  • Avoid bulky code repetition by creating utility functions, eg a setAudioTime() function.
  • Classes 'active' and 'playing' need to be added/removed independently of each other, or there's not point having both. Suggest adding "active" unconditionally and toggling "playing".
  • Should clicks on the $('.play') buttons affect the state of the $('.play2') buttons/audio and vice versa?

Small ideas

  • Brackets and braces out of balance. Assumption needed.
  • Avoid bulky tests by better exploiting jQuery selectors.
  • .removeClass()'s can address multiple classes in one hit.
  • Use .on() not .bind().
$(document).ready(function() {
    // *** Utility functions ***
    function secondsTimeSpanToHMS(s) {
        // this function is not mine
        var m = Math.floor(s / 60); //Get remaining minutes
        s -= m * 60;
        return (m < 10 ? '0' + m : m) + ":" + (s < 10 ? '0' + Math.floor(s) : Math.floor(s)); //zero padding on minutes and seconds
    }
    function setAudioTime(ev) {
        this.currentTime = this.duration * (ev.clientX - $(ev.target).offset().left) / $("#progcont").width();
    };

    // INDEX PLAYER
    $('.play').not(".master").on('click', function() {
        var $this = $(this);
        // Make the master a proxy for this play button
        var $master = $('.master').off('click').on('click', function() {
            $this.click();
        }).addClass('active');
        //stop currently playing audio and remove classes playing/active from all play buttons, exp this and master.
        $(".play").not(this).not($master).find(".playing").click().end().removeClass('playing active'); // probably?
        /* Here, control state of .play2 buttons/audio ??? */  
        $this.addClass('active').toggleClass('playing');
        var id = $this.attr('id').replace(/btn/, '');
        var audio = $('audio[id^="sound"]')[id - 1];
        if ($this.is('.playing')) {
            audio.play();
            $master.addClass('playing');
            $('#songlength').text(secondsTimeSpanToHMS(audio.duration));
        } else {
            audio.pause();
            $master.removeClass('playing');
        }
        $('#slider').off('click').on('click', setAudioTime.bind(audio));
    });

    //INDEX.HTML BIG PLAY BUTTON
    //All this code disappears because the master is now a proxy for the currently active play button.

    //PLAY ON MUZYKA
    $('.play2').on('click', function() {
        var $this = $(this);
        var audio = $this.find('audio').get(0);
        $(".play2").not(this).find(".playing").click().end().removeClass('playing active'); // probably?
        /* Here, control state of .play buttons/audio ??? */  
        $this.addClass('active').toggleClass('playing');
        $('#bottomPlayer').css('bottom', '0px');
        if ($this.hasClass('playing')) { //LAUNCHES THE TRACK
            audio.play();
            audio.currentTime = 0;
            $('#songlength').text(secondsTimeSpanToHMS(audio.duration));
        } else {
            $this.removeClass('playing');
            audio.pause();
            $('#bottomPlayer').css('bottom', '-50px');
        }
        $('#slider').off('click').on('click', setAudioTime.bind(audio));
    });
    $('#pauseAll').on('click', function() {
        $(".play2.playing").click();
    });
    $('audio[id^="sound"]').add('.play2 audio').get().ontimeupdate = function setProgess() {
        $('.progress').css('width', (this.currentTime / this.duration * 100 + '%'));
        $('#duration').text(secondsTimeSpanToHMS(this.currentTime));
    };
});

Not sure this is 100% correct. Heaps af assumptions were necessary, so will almost undoubtedly need debugging.

You may well choose to adopt some of these ideas and not others.

\$\endgroup\$
0
\$\begingroup\$

A few things not pointed out in the above answers:

1) You should place your code in a closure by creating an IIFE. This will create a private scope for your code so that you don't pollute the global scope.

(function( $ ) {
  //code here
})( jQuery );

In this example, I pass $ in so that jQuery can be safely referenced using it throughout your code.

2) Use 'use strict';. Strict mode helps prevent a lot of common issues.

(function( $ ) {
  'use strict';
  //code here
})( jQuery );

3) Cache your jQuery selections. Selecting elements is one of the slowest things that jQuery does. So creating variables at the top of your code to hold all of your selections. That way you can refer to this variable everywhere in your code.

(function( $ ) {
  'use strict';
  var $master = $('.master');
  //code here
})( jQuery );

4) You should create named functions instead of using anonymous functions. This will help with debugging via stack traces. Anonymous functions are much harder to debug.

(function( $ ) {
  'use strict';
  var $master = $('.master');
  function masterClickHandler() {
   //code here
  }
  $master.on('click', masterClickHandler);
})( jQuery );

5) You don't need all of your code in document.ready. If you follow the above advice, you can just call your named functions that have already been created. This is another slight performance boost.

(function( $ ) {
  'use strict';
  var $master;
  function masterClickHandler() {
   //code here
  }
  $(function(){  //shortcut for document.ready
    $master = $('.master');
    $master.on('click', masterClickHandler);
  });
})( jQuery );

Hope that helps.

\$\endgroup\$
  • \$\begingroup\$ Thank you for the help. I'll try my best to fix it. Still wonderin what practicly the 'use strict' help me gain. From what I've read it makes the code better at detecting problems, but not much apart from that. \$\endgroup\$ – aln447 Dec 8 '15 at 20:13
  • \$\begingroup\$ How is detecting problems in your code not helpful?? For example, It can throw an error if forgot to var a variable instead of creating a new variable and you wonder why your code breaks because nothing "looks" wrong. Did you read the article I linked to above? \$\endgroup\$ – Gary Storey Dec 8 '15 at 21:32
  • \$\begingroup\$ The OP's code is already wrapped in $(document).ready(function() {...}); therefore doesn't need another protective wrapper. If there's any concern is over the reliability of $, then simply change the wrapper to jQuery(function($) {...});. And yes, 'use strict' is a sensible precaution. \$\endgroup\$ – Roamer-1888 Dec 9 '15 at 1:15
  • \$\begingroup\$ One of my points was that not all code needs to be in document.ready. By adding the closure, the OP can set up all the functionality possible ahead of time. Only some code requires the ready event to have run for it to be successful (usually DOM selections, and events). \$\endgroup\$ – Gary Storey Dec 9 '15 at 3:18
  • \$\begingroup\$ Gary, Jesus, dude chill! I didn't say I don't find it usefull, I was just wondering about it's practical uses. No need to yell out of no where, I'm a beginner, so some things seem bizarre and complicated to me. Damn. I thought going to stackexchange would help me escape from such things... \$\endgroup\$ – aln447 Dec 9 '15 at 22:14

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service, privacy policy and cookie policy

Not the answer you're looking for? Browse other questions tagged or ask your own question.