3
\$\begingroup\$

This is a jQuery time update events for a video player with multiple ids. I feel the code is too long and I need to add more ids to fire events.

 $(document).ready(function() {
           var i = 0; 
           var a = 0; 
           var b = 0; 
           var c = 0; 
           var j = 0;

           $("#r-news-week1, #r-news-week2").bind("play", function() {
                if (i < 1) {
                    console.log("Video Started");
                    ga('send', 'event', 'Videos', 'Started', 'Video');
                }
                i = i + 1;
            });

             $("#r-news-week1, #r-news-week2").bind("timeupdate", function() {
                  var currentTime = this.currentTime;
                  if (currentTime > 0.75 * (this.duration)) {
                    if (c < 1) {
                      console.log("watched 75%");
                      ga('send', 'event', 'Videos', 'Watched 75%', 'Video');
                    } c = c + 1; 
                  } else if (currentTime > 0.50 * (this.duration)) {
                      if (b < 1) {
                      console.log("watched 50%");
                      ga('send', 'event', 'Videos', 'Watched 50%', 'Video');
                    } b = b + 1;
                  } else if (currentTime > 0.25 * (this.duration)) {
                      if (a < 1) {
                      console.log("watched 25%");
                      ga('send', 'event', 'Videos', 'Watched 25%', 'Video');
                    } a = a + 1;
                  }
              });


            $("#r-news-week1, #r-news-week2").bind("ended", function() {
                if (j < 1) {
                    console.log("Finished 100%");
                    ga('send', 'event', 'Videos', 'Finished 100%', 'Video');
                }
                j = j + 1;
            });

        })
\$\endgroup\$
  • \$\begingroup\$ Variable names i, a, ... bind is deprecated, use on. Cache $("#r-news-week1, #r-news-week2"), Chain methods on same object. For play and ended, the if conditions are not required as these events will fire only once, you can use one() to bind such events. \$\endgroup\$ – Tushar Dec 20 '16 at 8:45
1
\$\begingroup\$

If you can us the timeupdate event to detect start and finish events you could use a single event listener block. You could also put the events an an array and increment the current position in the video being listened for. Something similar to this:

function bindVideoEvents(ids) {
    var _events = [
        { time: 0.00, name: 'Started' },
        { time: 0.25, name: '25% Watched' },
        { time: 0.50, name: '50% Watched' },
        { time: 0.75, name: '75% Watched' },
        { time: 1.00, name: 'Finished 100%' }
    ];
    var _evPos = {};

    $.each(ids, function (idx, id) {
        _evPos[idx] = 0;
        $('#' + id).on('timeupdate', function () {
            var currentEvent = _events[_evPos[idx]];
            if (this.currentTime > (currentEvent.time * this.duration)) {
                console.log(currentEvent.name);
                ga('send', 'event', 'Videos', currentEvent.name, 'Video');
                _evPos[idx]++;
            }
        });
    });
} 
\$\endgroup\$
  • \$\begingroup\$ How about have _events outside the function (like a const), and maybe call it something like PROGRESS_PHASES? \$\endgroup\$ – Mario Santini Dec 23 '16 at 12:25
  • \$\begingroup\$ I don't think there a reason to move them outside the function unless they are needed elsewhere. I like PROGRESS_PHASES or PHASES better for a name. I was having trouble coming up with a good name for that constant. \$\endgroup\$ – backpackcoder Dec 23 '16 at 13:06
  • \$\begingroup\$ The reason to move outside the function is that you could want to configure your phases, and more you don't have to create the variable each time you call the function. \$\endgroup\$ – Mario Santini Dec 23 '16 at 13:15
  • \$\begingroup\$ It depends on how many times he function needs called. If it only is called once then there is no inefficiency. If the phases are not needed outside the function I'm find with the negligible performance hit to make the code more readable by keeping all the phases and events in the same location. It it were necessary to configure the phases or call the method a large number of time I would the consider moving them out. Until them I prefer to follow YANGI. \$\endgroup\$ – backpackcoder Dec 23 '16 at 18:38

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.