Refactoring repeated code in javascript prototype constructor

Question

I have a open function that once triggered, simply plays video in a dedicated panel.

This function can be triggered in two ways - one with a click and another one with a page load (window load) with url that contains a valid anchor tag.

They all work fine but some codes of the window load handler are repetitive and I'm not too sure how I can keep this DRY.

Please take a look and point me in some directions on how I can write this better. I commented in open function which is for which.

$.videoWatch.prototype = {
  init: function() {
    this.$openLinks = this.$element.find(".open");
    this.$closeLinks = this.$element.find(".close");
    this.open();
    this.close();
  },
  _getContent: function(element) {
    var $parent = element.parent(),
        id = element.attr('href').substring(1),
        title = $parent.data('title'),
        desc = $parent.data('desc');

        return {
          title: title,
          desc: desc,
          id: id
        }
  },
  open: function() {

    var self = this;

    //open theatre with window load with #hash id
    window.onload = function() {
        var hash = location.hash;

        var $a = $('a[href="' + hash + '"]'),
            content = self._getContent($a),
            $li = $a.parents("li"),
            $theatreVideo = $(".playing"),
            $theatreTitle = $(".theatre-title"),
            $theatreText = $(".theatre-text");

        $(".theatre").attr('id', content.id);
        $theatreTitle.text(content.title);
        $theatreText.text(content.desc);

        if ($theatreText.text().length >= 90) {
          $(".theatre-text").css({
            'overflow': 'hidden',
            'max-height': '90px',
          });
          $moreButton.insertAfter($theatreText);
        }

        $a.parent().addClass("active");
        $(".theatre").insertAfter($li);
        $(".theatre").slideDown('fast', scrollToTheatre);
        oldIndex = $li.index();

    }

    //open theatre with click event
    self.$openLinks.on("click", function(e) {
      // e.preventDefault();
      if (curID == $(this).parent().attr("id")) {
        $("figure").removeClass("active");
        $("button.more").remove();
        $(".theatre").slideUp('fast');
        $('.playing').attr("src", "");
        removeHash();
        oldIndex = -1;
        curID = "";
        return false
      } else {
        curID = $(this).parent().attr("id");
      }

      var $a = $(this),
          content = self._getContent($a),
          $li = $a.parents("li"),
          $theatreVideo = $(".playing"),
          $theatreTitle = $(".theatre-title"),
          $theatreText = $(".theatre-text");


      $(".theatre").attr('id', content.id);
      $theatreTitle.text(content.title);
      $theatreText.text(content.desc);

      if ($theatreText.text().length >= 90) {
          $(".theatre-text").css({
            'overflow': 'hidden',
            'max-height': '90px',
          });
          $moreButton.insertAfter($theatreText);
      }

      if (!($li.index() == oldIndex)) {
        $("figure").removeClass("active");
        $(".theatre").hide(function(){
          $a.parent().addClass("active");
          $(".theatre").insertAfter($li);
          $(".theatre").slideDown('fast', scrollToTheatre);
          oldIndex = $li.index();
        });
      } else {
        $(".theatre").insertAfter($li); 
        scrollToTheatre();
        $("figure").removeClass("active");
        $a.parent().addClass("active");
      } 
    });
  },

  ...

don't know if this helps you but you can make use of an array and for each element execute the code. and have 2 different triggers who trigger the function? it would atleast reduce writing the same function over and over. — Max
– Max, Commented Jan 24, 2016 at 22:52

RomanPerekhrest · Accepted Answer · 2016-01-24 23:26:29Z

Simplified and refactored open method:

open: function() {

    var self = this;
    var serviceObj = {
        theatreVideo : $(".playing"),
        theatre: $(".theatre"),
        theatreTitle : $(".theatre-title"),
        theatreText : $(".theatre-text"),
        setTheatreContent: function(content){
            this.theatre.attr('id', content.id);
            this.theatreTitle.text(content.title);
            this.theatreText.text(content.desc);

            if (this.theatreText.text().length >= 90) {
               this.theatreText.css({
                   'overflow': 'hidden',
                   'max-height': '90px',
               });
               $moreButton.insertAfter(this.theatreText);
            }
        },
      activateTeatre: function(a, li){
          a.parent().addClass("active");
          this.theatre.insertAfter(li);
          this.theatre.slideDown('fast', scrollToTheatre);
          oldIndex = li.index();
      }

    };

    //open theatre with window load with #hash id
    window.onload = function() {
        var hash = location.hash;
        var $a = $('a[href="' + hash + '"]'),
            content = self._getContent($a),
            $li = $a.parents("li");

       serviceObj.setTheatreContent(content);
       serviceObj.activateTeatre($a, $li);

    }

    //open theatre with click event
    self.$openLinks.on("click", function(e) {
      // e.preventDefault();
      if (curID == $(this).parent().attr("id")) {
        $("figure").removeClass("active");
        $("button.more").remove();
        $(".theatre").slideUp('fast');
        $('.playing').attr("src", "");
        removeHash();
        oldIndex = -1;
        curID = "";
        return false
      } else {
        curID = $(this).parent().attr("id");
      }

      var $a = $(this),
          content = self._getContent($a),
          $li = $a.parents("li");

      serviceObj.setTheatreContent(content);

      if (!($li.index() == oldIndex)) {
        $("figure").removeClass("active");
        $(".theatre").hide(function(){
          serviceObj.activateTeatre($a, $li);
        });
      } else {
        $(".theatre").insertAfter($li); 
        scrollToTheatre();
        $("figure").removeClass("active");
        $a.parent().addClass("active");
      } 
    });
  },

Gavriel · Accepted Answer · 2016-01-24 22:50:17Z

1

1st of all there are variables that don't depend on the input, you could pull them to the class (I'll show just one example, as you asked for directions):

init: function() {
  this.$theatreVideo = $(".playing");

All the variables that do depend on the input, like $li could be moved to a function:

  var $a = $(this),
      $dependsOnA = self.dependsOnA($a);
  self.actionDependsOnA($dependsOnA); // see below



function dependsOnA($a) {
    return {
      a: $a,
      li: $a.parents("li"),
      content: self._getContent($a)
    }
}

Also the code that "repeats" can be moved to a function:

function actionDependsOnA($dependsOnA)
    $(".theatre").attr('id', $dependsOnA.content.id);
    $theatreTitle.text($dependsOnA.content.title);
    $theatreText.text($dependsOnA.content.desc);
}

answered Jan 24, 2016 at 22:50

Gavriel

19.3k13 gold badges73 silver badges116 bronze badges

1 Comment

Seong Lee Over a year ago

Thank you. I have not thought of separating variables by input dependency. This will definitely help!

Collectives™ on Stack Overflow

Refactoring repeated code in javascript prototype constructor

2 Answers 2

Comments

1 Comment

Your Answer

Hot Network Questions

Collectives™ on Stack Overflow

2 Answers 2

Comments

1 Comment

Your Answer

Sign up or log in

Post as a guest

Related