2
\$\begingroup\$

This question is to see if I can get some input on the "design pattern" I tried to implement here. I'm just learning about closure in JavaScript and I think I'm starting to get it. I'm wondering if the way I wrote this code is or is not terrible stylistically.

The code in question is attempting to answer a challenge given in the JavaScript: The Hard Parts (by Will Sentance) on Frontend Masters. Based on the challenge, it seems I went above and beyond as the solution provided, when run, doesn't work properly.

The challenge:

Write a function changeColor that when invoked will first check if the current page background color is "rgb(221, 238, 255)". If it is, it changes the color to "rgb(255, 238, 221)". If it isn't, it sets the color to "rgb(221, 238, 255)".

Add a click event listener to button #1 above (it has an id of "activate"). On click, the button should log to the console "clicked 1". It should also set up a click event listener on button #2 (id of "color"). That listener should log to console "clicked 2" and then call the changeColor function you just created.

Clear the console and hit the 'Run with JS' button. Look at what code has run by analyzing the console. Then try to change the background color by clicking button #2. What needs to happen for the button to work?

Based on the wording of the challenge it seemed like the solution that was hinted at was (in pseudo-ish code):

// Provided solution to the question above 
// Assume all necessary HTML is in place

activationButton.addEventListener("click", () => {
  console.log("clicked activation button")
  colorChangerButton.addEventListener("click", () => {
    console.log("clicked color changer button")
    changeTheColor()
  })
})

The obvious problem with the code above is that a new event listener will be added to the colorChangerButton every time you click the activationButton. This was where I started thinking about trying to use a closure to keep track of a state variable so that I could make sure that the event listener only gets added to the second button once. Also, this is the solution provided in the solution set for the challenge.

To that end, this is what I came up with (please assume that the correct HTML is there, etc):

(function () {

  let clickCount = 0

  function changeColor() {
    if (document.body.style.backgroundColor === "rgb(221, 238, 255)") {
      document.body.style.backgroundColor = "rgb(255, 238, 221)"
    } else {
      document.body.style.backgroundColor = "rgb(221, 238, 255)"
    }
  }

  function activateButton2() {
    const btn2 = document.querySelector('#color') // Appropriate to grab button here?
    btn2.addEventListener("click", () => {
      console.log("clicked #2")
      changeColor()
    })
  }

  const btn1 = document.querySelector('#activate') // Appropriate to have variable declaration here?
  btn1.addEventListener("click", () => {
    clickCount++
    if (clickCount === 1) {
      activateButton2()
    } 
    console.log("clicked #1")
    return
  })
  // Initially had an un-necessary return statement here
})()

The code above - that I wrote - is working how I'd expect it to - the color change functionality and the console logs are firing as I would expect.

One question I had about my implementation was about containing the button variables inside the IIFE. I feel like I see a lot of front-end scripts putting all the query selectors at the top of the script - is there a reason to do that vs. including them within a function like this? If I understand closure correctly, once the event handlers are created there is a persistent reference to everything in the IIFE, so I can fire this function and the buttons and their handlers persist and behave as expected.

It also seems like I could probably use a boolean to track the "clicked" status of the first button, and a switch statement for checking the background color of the page.

Any other pointers anyone wants to offer?

\$\endgroup\$
  • 1
    \$\begingroup\$ Welcome to Code Review! Please clarify whether your code works or not? You said that it doesn't work properly, and then that it's working as you expect. \$\endgroup\$ – 200_success Jul 19 at 16:28
  • 1
    \$\begingroup\$ @200_success I will edit but the code that I wrote does work. I'm posting here to see if the approach I used is sensible/readable. I'll write some edits. \$\endgroup\$ – Anthony Jul 19 at 16:47
3
\$\begingroup\$

This is a partial answer, that only addresses style, but I prefer something like this for the changeColor function.

I'm not thrilled with activateColorButton, but it works.

This is much more readable and maintainable to me:

const changeColor = e => {
    const lightOrange = "rgb(221, 238, 255)"
    const lightBlue = "rgb(255, 238, 221)"
    const {style} = document.body
    style.backgroundColor = style.backgroundColor === lightOrange 
        ? lightBlue
        : lightOrange
}

const activateColorButton = e => {
    const btnColor = document.querySelector('#color');
    btnColor.removeEventListener('click', changeColor);
    btnColor.addEventListener('click', changeColor);
}

Some Explanation:

TLDR

  • Use good variable names
  • Avoid duplication
  • Use whitespace effectively
  • Prepare assignments for future abstraction

/TLDR

In changeColor, I have eliminated as much duplication as possible by assigning variables with names that are deliberately chosen to help the code to describe what it does without additional comments.

Moving the variable names to the top of the function allows them to be extended, changed, or even abstracted out to another function or data structure entirely.

The repeated reference to document.body.style.backgroundColor takes up half of a line of code, is very distracting, hard to read, hard to reason about, and unnecessary. style.backgroundColor could not be reduced to simply backgroundColor. This was as far down as it would go without losing functionality.

I personally find the compactness of the tertiary if statement to be more readable than the explicit if statement and especially prefer it when either assigning or returning one of 2 choices.

I renamed activateButton2 to activateColorButton b/c it more clearly expresses its purpose.

In activateColorButton, I chose to remove the click handler if it's already there and reassign it, rather than have confusing 'flag' variables lying around. I'm not thrilled with it b/c, ideally, it would check to see if the click handler was already assigned via the DOM directly, but I was too lazy to implement that here, which seems ok in this case.

\$\endgroup\$
  • 1
    \$\begingroup\$ Welcome to Code Review, thanks for improving your answer as requested! \$\endgroup\$ – Simon Forsberg Jul 22 at 10:01
  • \$\begingroup\$ Thank you for taking the time to write this out, this is exactly the sort of feedback I was looking for. I like that you got rid of the weird flag variable, and I like that everything is still contained within functions to keep the global namespace clean. It's readable and much more terse. This is the sort of code I'm looking to write! Would you recommend enclosing these functions inside an IIFE as in my first attempt? I am/was attempting to learn the "module" pattern as it pertains to using closure effectively and so I'm curious about whether that is a good approach or not. Thanks again! \$\endgroup\$ – Anthony Jul 22 at 16:26
  • 1
    \$\begingroup\$ @Anthony: There's really not too much to do as an IIFE here b/c you can just attach the first buttons handler through onclick in the DOM. If you do that, then try to put your js in an external file and use the IIFE to create a namespace for it, sort of like how jQuery does it. \$\endgroup\$ – Dov Rine Jul 22 at 17:24
  • 1
    \$\begingroup\$ I think you're probably better off coming up with a less trivial example to practice the module pattern on. Something like a todo list or a table with add/delete row functionality. \$\endgroup\$ – Dov Rine Jul 22 at 17:25

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service, privacy policy and cookie policy

Not the answer you're looking for? Browse other questions tagged or ask your own question.