Can this function be organized better?

How can this function be organized better?

https://jsfiddle.net/3p5vadjy/5/

Code 1 adds a delay to the fadeout

   setTimeout(function() {
      const fadeout = document.querySelector(".fadeout");
      fadeout.classList.add("fade");
    }, 5000);

Code 2 adds a delay to .split-wrap being hidden from the dom

      const splitwrap = document.querySelector(".split-wrap");
      splitwrap.classList.add("hide");
    }, 10000);

Code 3 While the curtains open, you are able to click the play button on the YouTube video.

const splitWrap = document.querySelector(".split-wrap");
    splitWrap.style.pointerEvents = "none"

Full Function


  function coverClickHandler(evt) {
    const cover = evt.currentTarget;
    hide(cover);
    const curtain = document.querySelector(".curtain");
    curtain.classList.add("slide");
    setTimeout(function() {
      const fadeout = document.querySelector(".fadeout");
      fadeout.classList.add("fade");
    }, 5000);
    setTimeout(function() {
      const splitwrap = document.querySelector(".split-wrap");
      splitwrap.classList.add("hide");
    }, 10000);
        const splitWrap = document.querySelector(".split-wrap");
    splitWrap.style.pointerEvents = "none";
    const thewrap = curtain.parentElement.querySelector(".container");
    show(thewrap);
  }

Do you remember how I was saying earlier that handlers should get information from the event, and then pass that event information on to other functions?

  function showVideo(cover) {
    hide(cover);
    const curtain = document.querySelector(".curtain");
    curtain.classList.add("slide");
    setTimeout(function() {
      const fadeout = document.querySelector(".fadeout");
      fadeout.classList.add("fade");
    }, 3000);
    setTimeout(function() {
      const splitwrap = document.querySelector(".split-wrap");
      splitwrap.classList.add("hide");
    }, 9000);
        const splitWrap = document.querySelector(".split-wrap");
    splitWrap.style.pointerEvents = "none";
    const thewrap = curtain.parentElement.querySelector(".container");
    show(thewrap);
  }

  function coverClickHandler(evt) {
    const cover = evt.currentTarget;
    showVideo(cover);
  }

That helps to clean up the handler, and moves less of the trouble into the showVideo function.

Let’s now tidy up the showVideo function by moving parts out to suitably named functions too.

  function slideCurtain() {
    const curtain = document.querySelector(".curtain");
    curtain.classList.add("slide");
  }
  
  function delayFadeout() {
    setTimeout(function() {
      const fadeout = document.querySelector(".fadeout");
      fadeout.classList.add("fade");
    }, 3000);
  }

  function delayHide(el) {
      setTimeout(function() {
      el.classList.add("hide");
    }, 9000);
  }
  
  function initSplitWrap() {
    const splitWrap = document.querySelector(".split-wrap");
    splitWrap.style.pointerEvents = "none";
    delayFadeout();
    delayHide(splitWrap);
  }
  
  function showWrap(curtain) {
    const thewrap = curtain.parentElement.querySelector(".container");
    show(thewrap);
  }

The showVideo function is now much easier to understand.

  function showVideo(cover) {
    hide(cover);
    slideCurtain();
    initSplitWrap();
    showWrap(curtain);
  }
1 Like

I’m getting an error.

curtain is not defined"

https://jsfiddle.net/p45a1tkz/2/

This is where learning can occur.

The showWrap function uses curtain. Moving it into where curtain is defined makes good sense.

  function showWrap(curtain) {
    const thewrap = curtain.parentElement.querySelector(".container");
    show(thewrap);
  }

  function slideCurtain() {
    const curtain = document.querySelector(".curtain");
    curtain.classList.add("slide");
    showWrap(curtain);
  }

That showWrap function doesn’t need to search for thewrap. If thewrap is assigned in the slideCurtain function we can just use the show function instead.

  // function showWrap(curtain) {
  //   const thewrap = curtain.parentElement.querySelector(".container");
  //   show(thewrap);
  // }

  function slideCurtain() {
    const curtain = document.querySelector(".curtain");
    curtain.classList.add("slide");
    const thewrap = curtain.parentElement.querySelector(".container");
    show(thewrap);
  }

That leaves us with the code at https://jsfiddle.net/pmw57/0L73qvfc/

This:


   setTimeout(function() {
      const splitwrap = document.querySelector(".split-wrap");
      splitwrap.classList.add("hide");
    }, 10000);

Goes with this:

  function delayHide(el) {
    setTimeout(function() {
      el.classList.add("hide");
    }, 9000);

}

It doesn’t work when it’s not added.

I changed this

  function delayHide(el) {
      setTimeout(function() {
      el.classList.add("hide");
    }, 9000);
  }
  
  function initSplitWrap() {
    const splitWrap = document.querySelector(".split-wrap");
    splitWrap.style.pointerEvents = "none";
    delayFadeout();
    delayHide(splitWrap);
  }

To this updated one

Was I not supposed to do that?

Updated:
https://jsfiddle.net/6r41q3ce/3/

   function delayHideSplitWrap() {
    setTimeout(function() {
      const splitwrap = document.querySelector(".split-wrap");
      splitwrap.classList.add("hide");
    }, 8000);
  }

  function initSplitWrap() {
    const splitWrap = document.querySelector(".split-wrap");
    splitWrap.style.pointerEvents = "none";
    delayFadeout();
    delayHideSplitWrap();
  }

That seems good. It’s nice that extracting code out to separate small functions helps to make it easier to organise things.

My mistake, I didn’t realize you made an improvement here by removing duplication.

  function delayHide(el) {
      setTimeout(function() {
      el.classList.add("hide");
    }, 9000);
  }
  
  function initSplitWrap() {
    const splitWrap = document.querySelector(".split-wrap");
    splitWrap.style.pointerEvents = "none";
    delayFadeout();
    delayHide(splitWrap);
  }

In this code, what does this do?

hide(cover);

With it removed, the code still works.

Does it serve a purpose, or can it be removed?

https://jsfiddle.net/chgtb2pq/

  function showVideo(cover) {
    //hide(cover);
    slideCurtain();
    initSplitWrap();
  }

As the code still works without it, it seems that you have found some dead code.

Removing that dead code is an important part of keeping your code clean.

2 Likes