Can this function be organized better?

JavaScript
1

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);
  }
2

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
4

I’m getting an error.

curtain is not defined"

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

5

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/

6

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.

7

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();
  }
8

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

9

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);
  }
10

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();
  }
11

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
12

I do not think having everything lumped together like this is a good idea, and should be avoided.
https://jsfiddle.net/6uyahwbg/

  function userAgent() {
    const theBody = document.querySelector("body");
    theBody.classList.remove("bg1");
    const theActive = document.querySelector(".with-curtain.active");
    theActive.classList.remove("active");
    const theHides = document.querySelectorAll(".hide");
    theHides.forEach(function needsAName(removeHide) {
      removeHide.classList.remove("hide");
    });

  }

So, I separated them into separate functions here:

Is the flow of how these functions are organized with separation good?

Is there anything I should change or adjust?
https://jsfiddle.net/e7a304df/

Does that look right?
or should those be in a different order?

1st function showHidden() {

2nd function showBody() {

3rd function showActive() {

Should showActive be under showHidden instead?

I’m a little confused on what the order of those should be.

That would look like this then:
https://jsfiddle.net/gjuybfed/

function showHidden() {
    const theHides = document.querySelectorAll(".hide");
    theHides.forEach(function needsAName(removeHide) {
      removeHide.classList.remove("hide");
    });

  }

  function showActive() {
    const theActive = document.querySelector(".with-curtain.active");
    theActive.classList.remove("active");
  }

  function showBody() {
    const theBody = document.querySelector("body");
    theBody.classList.remove("bg1");
    showHidden();
    showActive();
  }

  function homeClickHandler() {
    showBody();
  }

I was thinking about doing it this way:

Would this way be better?
https://jsfiddle.net/1zeh93ko/

  function showHidden() {
    const theHides = document.querySelectorAll(".hide");
    theHides.forEach(function needsAName(removeHide) {
      removeHide.classList.remove("hide");
    });

  }

  function showBody() {
    const theBody = document.querySelector("body");
    theBody.classList.remove("bg1");
    const theActive = document.querySelector(".with-curtain.active");
    theActive.classList.remove("active");
    showHidden();
  }

  function homeClickHandler() {
    showBody();
  }

I also don’t know what to call this function: It needs a name.
theHides.forEach(function needsAName(removeHide) {

With this one

I changed this:

  function showCovers(playButton) {
    const cover = playButton.parentElement;
    cover.classList.add("active");
    show(cover);
    const theBody = document.querySelector("body");
    theBody.classList.add("bg1");
  }

To this:

const manageCover = (function makeManageCover() {

  function showBody() {
    const theBody = document.querySelector("body");
    theBody.classList.add("bg1");
  }

  function showCovers(playButton) {
    const cover = playButton.parentElement;
    cover.classList.add("active");
    show(cover);
    showBody();
  }

  function coverClickHandler(evt) {
    hideAll(config.containers);
    const cover = evt.currentTarget;
    showCovers(cover);
  }

Though, I probably could have left it as this:

 function showCovers(playButton) {
    const cover = playButton.parentElement;
    cover.classList.add("active");
    show(cover);
    const theBody = document.querySelector("body");
    theBody.classList.add("bg1");
  }

What do you think?

13

or, maybe like this would be better?

The showHidden function should be first, or last?

Like this?
https://jsfiddle.net/2v05e6so/

 function showHidden() {
    const theHides = document.querySelectorAll(".hide");
    theHides.forEach(function needsAName(removeHide) {
      removeHide.classList.remove("hide");
    });

  }

  function showActive() {
    const theActive = document.querySelector(".with-curtain.active");
    theActive.classList.remove("active");
    const theBody = document.querySelector("body");
    theBody.classList.remove("bg1");
    showHidden();
  }

  function homeClickHandler() {
    showActive();
  }

or like this?

I think this may look better.
https://jsfiddle.net/s5zyjv17/

This is all confusing to me.

function showActive() {
    const theActive = document.querySelector(".with-curtain.active");
    theActive.classList.remove("active");
    const theBody = document.querySelector("body");
    theBody.classList.remove("bg1");
  }

  function showHidden() {
    const theHides = document.querySelectorAll(".hide");
    theHides.forEach(function needsAName(removeHide) {
      removeHide.classList.remove("hide");
    });
    showActive();

  }

  function homeClickHandler() {
    showHidden();
  }
14

I am not giving advice on many different branches of code.

Please pick one set of code that you feel the best about, and we’ll work on making progress from there.

1 Like
15

I do not think having everything lumped together like this is a good idea, and should be avoided.

How can we make progress from here creating separation?

https://jsfiddle.net/dtneahsm/

  function userAgent() {
    const theActive = document.querySelector(".with-curtain.active");
    theActive.classList.remove("active");
    const theBody = document.querySelector("body");
    theBody.classList.remove("bg1");
    theBody.classList.remove("bg2");
    theBody.classList.remove("bg3");
    const theHides = document.querySelectorAll(".hide");
    theHides.forEach(function needsAName(removeHide) {
      removeHide.classList.remove("hide");
    });

  }

I’m thinking each of these would be in their own function if you agree.

In what order the flow of the functions would be arranged in, I’m not sure.

Meaning, which comes 1st, 2nd, 3rd.

    const theActive = document.querySelector(".with-curtain.active");
    theActive.classList.remove("active");

    const theBody = document.querySelector("body");
    theBody.classList.remove("bg1");
    theBody.classList.remove("bg2");
    theBody.classList.remove("bg3");

    const theHides = document.querySelectorAll(".hide");
    theHides.forEach(function needsAName(removeHide) {
      removeHide.classList.remove("hide");
});

Here are function names I picked out if you think those are good and can be used.

function showActive();
function showBody();
function showHidden();

And here if you think it is necessary and makes sense to.

  function showCovers(playButton) {
    const cover = playButton.parentElement;
    cover.classList.add("active");
    show(cover);
    const theBody = document.querySelector("body");
    theBody.classList.add("bg1");
    theBody.classList.add("bg2");
    theBody.classList.add("bg3");
  }

Meaning, if you think it would be good to move this out into its own function:

    const theBody = document.querySelector("body");
    theBody.classList.add("bg1");
    theBody.classList.add("bg2");
    theBody.classList.add("bg3");

Like this:

  function showBody() {
    const theBody = document.querySelector("body");
    theBody.classList.add("bg1");
    theBody.classList.add("bg2");
    theBody.classList.add("bg3");
  }
16

I’m first drawn to the name of userAgent. Why is it called that? It doesn’t seem to make much sense when it’s named like that. We can figure out a better name for it by considering what it does.

What happens when you click the cross, is that everything is reset back to the initial state. Calling the cross home is also confusing too. Normally a cross is to close something. If it was to go home then it should be a house symbol instead.

However, for some reason you have chosen to call the X symbol home instead. That to me is a much larger problem to be fixed. Being consistent about home or close is the most important issue at hand.

After that we can work on useragent, and after that we can work on what’s inside of it.

2 Likes
17

About userAgent, I have no idea, I couldn’t come up with a good name to call it. I googled user interface, then came across user agent and I went with that name.

I’m not really sure what a good name to call that would be.

The X, it’s meant to exit out of the video/page/screen. That’s its main purpose, to exit out and go back to the previous screen/page.

Maybe it should be called Exit, that or close.

https://jsfiddle.net/dtneahsm/

About what this function does, @PaulOB helped me with it.

He would know more about what .theActive and .theHides do in the code.

He explains what those do in here.

That function was made in this Post.

In the post he says this:

  1. Loop through the home buttons and add a click handler.

  2. When the home button is clicked remove the active class and the hide classes and them toggle an animation class on the body.

 function userAgent() {
    const theActive = document.querySelector(".with-curtain.active");
    theActive.classList.remove("active");
    const theBody = document.querySelector("body");
    theBody.classList.remove("bg1");
    theBody.classList.remove("bg2");
    theBody.classList.remove("bg3");
    const theHides = document.querySelectorAll(".hide");
    theHides.forEach(function needsAName(removeHide) {
      removeHide.classList.remove("hide");
    });

These are the backgrounds:

Each svg button has its own background.

    const theBody = document.querySelector("body");
    theBody.classList.remove("bg1");
    theBody.classList.remove("bg2");
    theBody.classList.remove("bg3");

If you think Close is a better name, that works good.

I changed it to Close here:
https://jsfiddle.net/ybkum9tr/3/

 function closeClickHandler() {
    userAgent();
  }

  function addClickToClose(closeButton) {
    closeButton.forEach(function closeButtonHandler(closeButton) {
      closeButton.addEventListener("click", closeClickHandler);
    });
  }

  function init() {
    const closeButton = document.querySelectorAll(".close");
    addClickToClose(closeButton);
  }
18

If you were to give someone a broad overview of what it does, in less than 10 words, what would you say? I might say something like: “It resets things back to how they started”. That’s 8 words. We get the idea of reset from that. That can be summarised down to a function name of resetPage. Instead of userAgent, it’s much easier to understand what resetPage does.

Close tends to imply that there is a separate Open somewhere else, so it’s better to go with Exit instead.

That’s alright - we can get to that after the other more distracting issues have been taken care of.

1 Like
19

I made all the necessary changes you asked me to make.

Added: function resetPage()

And changed Close to Exit.

https://jsfiddle.net/hfkc2Lw9/

const manageUI = (function makeManageUI() {

  function resetPage() {
    const theActive = document.querySelector(".with-curtain.active");
    theActive.classList.remove("active");
    const theBody = document.querySelector("body");
    theBody.classList.remove("bg1");
    const theHides = document.querySelectorAll(".hide");
    theHides.forEach(function needsAName(removeHide) {
      removeHide.classList.remove("hide");
    });

  }

 function exitClickHandler() {
    resetPage();
  }

  function addClickToExit(exitButton) {
    exitButton.forEach(function addExitButtonHandler(exitButton) {
      exitButton.addEventListener("click", exitClickHandler);
    });
  }

  function init() {
    const exitButton = document.querySelectorAll(".exit");
    addClickToExit(exitButton);
  }

  return {
    init
  };
}());

Moving ahead, if I understand this correctly, only the reset stuff should be inside that function, right?

This is an educated guess on my part, but I’m not sure.

Would this be done next?
https://jsfiddle.net/d29rwu35/

showBody would come 1st,
Then resetPage would come after that?

Body is the first thing you see, the reset occurs after viewing the body which is 2nd.

function showBody() {
    const theBody = document.querySelector("body");
    theBody.classList.remove("bg1");
  }

  function resetPage() {
    const theActive = document.querySelector(".with-curtain.active");
    theActive.classList.remove("active");
    const theHides = document.querySelectorAll(".hide");
    theHides.forEach(function needsAName(removeHide) {
      removeHide.classList.remove("hide");
    });
    showBody();
  }

  function exitClickHandler() {
    resetPage();
  }
20

Good one. Now about the code in that resetPage function.

There are three different sections that do quite different things in that code:

  • The curtain code hides the curtain.
  • The next part with the body resets the background.
  • And the part with theHides shows all of the players.

Suitable functions names for each of those are: hideAllCurtains, resetBackground, and showAllPlayers.

Each function should have parameters. At minimum, either a selector or the result of the querySelector should be given to each function.

As hideAllCurtains is using a selector, it’s usually more reliable to loop through all found elements and do something with them.

    function hideAllCurtains(curtainSelector) {
        const activeCurtains = document.querySelectorAll(curtainSelector + ".active");
        activeCurtains.map(function hideCurtain(curtain) {
            curtain.classList.remove("active");
        });
    }

    function resetPage() {
        hideAllCurtains(".with-curtain");
        ...
    }

In the hideAllCurtains function we can turn the querySelectorAll results into an array, so that we can use filter and map techniques.

    function hideAllCurtains(curtainSelector) {
        const allCurtains = Array.from(
            document.querySelectorAll(curtainSelector)
        );
        const activeCurtains = allCurtains.filter(function isCurtain(curtain) {
            return curtain.classList.contains(".active");
        });
        activeCurtains.map(function hideCurtain(curtain) {
            curtain.classList.remove("active")
        );
    }

We can then pull up those functions into arrow-notation, to help simplify the code:

    function hideAllCurtains(curtainSelector) {
        const allCurtains = Array.from(
            document.querySelectorAll(curtainSelector)
        );
        const isActive = (curtain) => curtain.classList.contains("active");
        const hideCurtain = (curtain) => curtain.classList.remove("active");
        allCurtains.filter(isActive).map(hideCurtain);
    }

The resetBackground function can be quite similar:

    function resetBackground(targetSelector) {
        const allTargets = Array.from(
            document.querySelectorAll(targetSelector)
        );
        const removeBackground = (target) => target.classList.remove("bg1");
        allTargets.map(removeBackground);
    }

    function resetPage() {
        ...
        resetBackground("body");
        ...
    }

    function showAllPlayers(selector) {
        const allPlayers = Array.from(
            document.querySelectorAll(selector)
        );
        const showPlayer = (player) => player.classList.remove("hide");
        allPlayers.map(showPlayer);
    }

    function resetPage() {
        hideCurtains(".with-curtain");
        resetBackground("body");
        showAllPlayers(".hide");
    }

There are some problems with the code though, it which I’ll get to in the next post.

1 Like
21

Now that we have a consistent structure to the functions, there are some issues that I notice with the showAllPlayers code.

The function parameter is just selector. It would be better if we found a more specific name for it as we did with the the other functions.

Another problem is that the “.hide” selector is far too generic too. That will easily cause problems with later development as it too easily catches everything that is hidden, not just the players that we care about.

To deal with that, we can investigate to find out what is hidden, and use more specific names to deal with them.

Here is what is hidden:

<div class="container play2 with-curtain hide">
...
<div class="container play3 with-curtain hide">

Those are the same containers that we used with hideCurtains, using the “.with-curtain” selector. That means we should be able to use that with the showAllPlayers function too.

    function resetPage() {
        hideCurtains(".with-curtain");
        resetBackground("body");
        showAllPlayers("..with-curtain");
    }

We should also group those “.with-curtain” parts together:

    function resetPage() {
        resetBackground("body");
        hideCurtains(".with-curtain");
        showAllPlayers(".with-curtain");
    }

And now it’s easy to tell that the resetPage code is only doing two main things instead of three. One thing is to reset the background, and the other is to reset the curtains where two different things are done to reset them.

We can have a separate function called just resetCurtains that does both of the things that are done in the hideCurtains and the showAllPlayers functions.

    function resetCurtains(curtainSelector) {
        const allCurtains = Array.from(
            document.querySelectorAll(curtainSelector)
        );
        const removeCurtain = (curtain) => curtain.classList.remove("active");
        const showPlayer = (player) => player.classList.remove("hide");
        allCurtains.map(removeCurtain);
        allCurtains.map(showPlayer);
    }

    function resetPage() {
        resetBackground("body");
        resetCurtains(".with-curtain");
    }

That’s a much better way to do things. https://jsfiddle.net/cnh0g7bu/

1 Like