What's the difference, and which way is better? Show/Remove classes

JavaScript
1

What’s the difference between doing this:
https://jsfiddle.net/dpavL376/

 (function manageCurtain() {
   "use strict";

   function coverClickHandler(evt) {
     const wrap = document.querySelector(".wrap");
     wrap.classList.remove("hide");
   }

And doing this?
https://jsfiddle.net/dpavL376/2/

 (function manageCurtain() {
   "use strict";

   function show(el) {
     el.classList.remove("hide");
   }

   function coverClickHandler(evt) {
     const thewrap = curtain.parentElement.querySelector(".wrap");
     show(thewrap);
   }
2

As with so many things, it depends.

In this case, it depends on whether you ever need to have the show() function available as a utility function in other areas of your code.

If you don’t need that, then the first example is equally valid, so long as it’s understood that it’s tied only to the .wrap class, and can’t be used for anything else, unless you refactored it.

1 Like
3

With the second example, work is being kept to a minimum in the handler function. As handlers tend to get messy quickly and easily, It’s best for handlers to offload the work to other functions.

1 Like
4

What would you say if someone asked you what happens when you click the cover? You would quite likely summarise what happens as “it shows the video”. That is the basis of what to name the function.

Here is the existing coverClickHandler function:

   function coverClickHandler(evt) {
     const cover = evt.currentTarget;
     hide(cover);
     const curtain = document.querySelector(".curtain");
     curtain.classList.add("slide");
     const thewrap = curtain.parentElement.querySelector(".wrap");
     show(thewrap);
   }

Three different things happen in that handler function.

  1. hide the cover
  2. slide the curtain
  3. show the video

We should then have code that is as simple as:

hide(cover);
slide(curtain);
showVideo(curtain);

We could even combine hide and slide into an openCurtain function, letting us realistically simplify things to this:

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

After an easy update to the code, we have achieved that simplicity.

Keeping it simple is about making it really easy to understand what the code does and why.

1 Like
5

Thank you for explaining that.

6

The video isn’t showing here:
https://jsfiddle.net/zxm4ghyv/

What did I do wrong?

(function manageCurtain() {
   "use strict";

   function show(el) {
     el.classList.remove("hide");

   }

   function hide(el) {
     el.classList.add("hide");
   }


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

   function showVideo(curtain) {
     const thewrap = curtain.parentElement.querySelector(".wrap");
     show(thewrap);
   }


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

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

   function showVideo() {
     openCurtain();
     initSplitWrap();
   }

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

   const covers = document.querySelectorAll(".jacketa");
   covers.forEach(function addHandler(el) {
     el.addEventListener("click", coverClickHandler);
   });
 }());
7

When asking for this kind of help, can you please also link us to the most recent working code for comparison, before things went wrong. Thanks.

1 Like
8

Last working version:
https://jsfiddle.net/mqspoyj5/

After adding the bottom:
https://jsfiddle.net/L3725t8z/

I tried adding this to it:

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

   function showVideo(curtain) {
     const thewrap = curtain.parentElement.querySelector(".wrap");
     show(thewrap);
   }


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

A problem is that the code already has a showVideo function.

To more easily combine the code, it helps to recognise that the showVideo function is doing multiple things that are not related to video.

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

We could rename that function to openCurtainsAndShowVideo(), but when there’s an “and” in the function name it helps to try and think of a more general term that encompasses both things. Something like startPlayingVideo(), and rename initSplitWrap to showVideo.

  // function initSplitWrap() {
  function showVideo() {
    ...
  }
  function startPlayingVideo(cover) {
    hide(cover);
    slideCurtain();
    showVideo();
  }
...
  function coverClickHandler(evt) {
    const cover = evt.currentTarget;
    startPlayingVideo(cover);
  }

The click handler function is now nice and simple.

Looking further up from the click handler function we have three methods in startPlayingVideo

 function startPlayingVideo(cover) {
    hide(cover);
    slideCurtain();
    showVideo();
  }

We can easily move hide and slideCurtain into their own openCurtain function.

 function openCurtain(cover) {
    hide(cover);
    slideCurtain(); 
 }
 
 function startPlayingVideo(cover) {
    openCurtain(cover);
    showVideo();
  }
10

We can now easily see that showVideo and slideCurtain have zero parameters. Usually that’s a sign of a bad function, because it’s not capable of working under different situations. Usually is best when a function has one or two function parameters.

With slideCurtain, the curtain variable should be moved outside of the function and passed in as a function parameter.

   function slideCurtain(curtain) {
     curtain.classList.add("slide");
     ...
   }
   ...
   function openCurtain(cover) {
     hide(cover);
     const curtain = document.querySelector(".outer");
     slideCurtain(curtain);
   }

The thewrap code in slideCurtain also should move out of the curtain code, and in to the showVideo code instead.

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

   function slideCurtain(curtain) {
     curtain.classList.add("slide");
     curtain.classList.add("fadeout");
   }
...
   function showVideo(curtain) {
     const thewrap = curtain.parentElement.querySelector(".wrap");
     show(thewrap);

We do need to pass curtain to the showVideo function, so in the startPlayingVideo function we can get the curtain and pass it from there.

   function openCurtain(cover) {
     ...
     return curtain;
   }
   ...
   function startPlayingVideo(cover) {
     const curtain = openCurtain(cover);
     showVideo(curtain);
   }

There’s still a few things to deal with in that code, such as making sense of splitwrap.

11

I’m not sure what the splitWrap code does, so that needs further investigation.

What can help though is to pass important variables to the code.

   function delayHideSplitWrap(splitWrap) {
     setTimeout(function() {
       splitWrap.classList.add("hide");
     }, 3000);
   }
...
   function showVideo(curtain) {
     ...
     const splitWrap = document.querySelector(".split-wrap");
     splitWrap.style.pointerEvents = "none";
     delayHideSplitWrap(splitWrap);
   }

It seems that the splitWrap stuff is a part of the cover, so we can move that to where it belongs.

   function openSplitWrap(splitWrap) {
     splitWrap.style.pointerEvents = "none";
     delayHideSplitWrap(splitWrap);
   }
...
   function openCurtain(cover) {
     hide(cover);
     const splitWrap = document.querySelector(".split-wrap");
     openSplitWrap(splitWrap);
     const curtain = document.querySelector(".outer");
     slideCurtain(curtain);
     return curtain;
   }

That code is a lot improved compared to how it began.

1 Like
12

.split-wrap is the play image that splits apart when it is clicked on.

13

Let’s move that out to a splitPlay function then.

   function splitPlay(splitWrap) {
     splitWrap.style.pointerEvents = "none";
     delayHideSplitWrap(splitWrap);
   }
...
   function startPlayingVideo(cover) {
     const splitWrap = document.querySelector(".split-wrap");
     splitPlay(splitWrap);
     const curtain = openCurtain(cover);
     showVideo(curtain);
   }
1 Like
14

How the differences can be told apart.

Play image that splits apart
This one doesn’t use (cover)

https://jsfiddle.net/vzbps8cm/

Play image that doesn’t split apart:
This one uses (cover)

https://jsfiddle.net/st2yr6c5/

15

Further improvements:

This can become:
https://jsfiddle.net/z93kwsqn/

 (function iife() {
   "use strict";

   function show(el) {
     el.classList.remove("hide");
   }

   function coverClickHandler(evt) {
     const wrapper = evt.currentTarget.parentElement;
     show(wrapper);
     videoPlayer.play();
   }
   const cover = document.querySelector(".play");
   cover.addEventListener("click", coverClickHandler);
 }());

This:
https://jsfiddle.net/ty926rgd/

(function iife() {
  "use strict";

  function coverClickHandler(evt) {
    videoPlayer.play();
  }

  const cover = document.querySelector(".play");
  cover.addEventListener("click", coverClickHandler);
}());
17

This topic was automatically closed 91 days after the last reply. New replies are no longer allowed.