CODEWITHSUNDEEP
javascript
Vikintas Raudavicius
Vikintas Raudavicius

Reputation: 102

How to make small snippet of code less repetitive?

I have two different video players in a page and right now they are working as intended, however, the code itself looks silly and unneccesarily repetitive, but I am not sure how to make it work while removing the basically copied block of code.

There are two divs with individual videos:

First

<video class='video2' poster='./assets/video2.jpg' src='./assets/video2.mp4'></video>
              <div class='buttons'>
                  <button id='playPause2'><img src='./assets/play.png'></button>
                </div>

Second

<video class='video1' poster='./assets/videoPoster.jpg' src='./assets/video1.mp4'></video>
      <div class='buttons'>
        <button id='playPause1'><img src='./assets/play.png'></button>
      </div>

And the JS that runs the videos is as follows 

var video1 = document.querySelector('.video1');
var video2 = document.querySelector('.video2');
var btn1 = document.getElementById('playPause1');
var btn2 = document.getElementById('playPause2');

function togglePlayPause1() {
  if (video1.paused) {
    btn1.className = 'pause';
    video1.play();
  } else {
    btn1.className = 'play';
    video1.pause();
  }
}

function togglePlayPause2() {
  if (video2.paused) {
    btn2.className = 'pause';
    video2.play();
  } else {
    btn2.className = 'play';
    video2.pause();
  }
}

btn1.onclick = function() {
  togglePlayPause1();
};

btn2.onclick = function() {
  togglePlayPause2();
};

As I said it works, but looks very silly, basically the same code repeating twice. How can I make this cleaner, while still holding the same functionality?

Upvotes: 0

Views: 46

Answers (3)

Digitrance
Digitrance

Reputation: 759

I have not tested it, but I think it should work. Can you give it a try and let me know?

[1, 2]
  .map(function(n) { return document.getElementById('playPause' + n); })
  .forEach(function(btn, i) {
    btn.onclick = function() {
      var video = document.querySelector('.video' + i);      
      btn.className = video.paused ? 'play' : 'pause';
      if (video.paused)     
        video.play();
      else
        video.pause();
    };
  });

However, I would suggest an alternative solution at the cost of brevity of solution.

I would suggest going for a proper naming convention when naming your buttons and videos. For instance, the IDs can be v1_video and v1_btn. Yes, I also recommend using IDs instead of class names for identifying different videos.

Following such a convention will make your code more readable and extensible for more complex use-cases like fetching the list of videos from an API.

['v1', 'v2'] // This array can instead be a list of IDs being fetched from an API
  .map(function(label) {
    return {
      Video: '#' + label + '_video',
      Button: '#' + label + '_btn'
    };
  })
  .forEach(function(pair, i) {
    var btn = document.querySelector(pair.Button);
    btn.onclick = function() {
      var video = document.querySelector(pair.Video);      
      btn.className = video.paused ? 'play' : 'pause';
      if (video.paused)     
        video.play();
      else
        video.pause();
    };
  });

Upvotes: 0

trincot
trincot

Reputation: 351403

You could pass the CSS selectors for both types of elements to a function. With some ternary operator, you can also reduce a bit the code:

function manageVideo(videoSelector, btnSelector) {
    let video = document.querySelector(videoSelector);
    document.querySelector(btnSelector).onclick = function() {
        this.className = video.paused ? "pause" : "play";
        video[video.paused ? "play" : "pause"]();
    };
}

manageVideo(".video1", "#playPause1");
manageVideo(".video2", "#playPause2");

Upvotes: 0

Fernando Siebra
Fernando Siebra

Reputation: 31

var video1 = document.querySelector('.video1');
var video2 = document.querySelector('.video2');
var btn1 = document.getElementById('playPause1');
var btn2 = document.getElementById('playPause2');

function togglePlayPause(video) {
  if (video.paused) {
    this.className = 'pause';
    video.play();
  } else {
    this.className = 'play';
    video.pause();
  }
}


var videos = [video1,video2];
[btn1,btn2].forEach(function(el,i){
  el.onclick = function()
  {
      togglePlayPause(videos[i]);
  }
})

Upvotes: 2

Related Questions