How to make my Javascript code even more better & cleaner?

[n_merciano]

Hello there great folks!

I am trying to become better in javascript i am starting to understand the concepts and the way things work. However i have been working on a simple Javascript snippet where you press keys it plays sound and adds a animation.

I would like to know if someone can tell me how i can simplify my code even more… and make it better looking and cleaner.

Would really appreciate it!

var audioController = (function() {

	const keys = document.querySelectorAll('.key');

	function playSound(e) {
		const audio = document.querySelector(`[audio-key="${e.keyCode}"]`);
		const key = document.querySelector(`[data-key="${e.keyCode}"]`);

		if(!audio) return; // Stop the function from running all together.
		audio.currentTime = 0; // Rewind to the start
		audio.play();

		key.classList.add('playing');
	}

	function removeTransition(e){

		[].forEach.call(keys, (key) => {
			key.classList.remove('playing');
		});

	}

	// Run the RunAudio function.
	document.addEventListener('keydown', playSound);
	document.addEventListener('transitionend', removeTransition);

})();

Thanks

[MartinMuzatko]

Hey there!

There are many options to make this a little more organized.
It is great that you use data-attributes to make creating keyboards a task of the HTML .

What catches my eye is how you loop over the keys to remove their Transition.

[].forEach.call(keys, (key) => {

why didn’t you use a for of loop?

for (key of keys) {
...
}

“Better” and “cleaner” is a individual and personal decision. While there are some “best practices” like pure functions, you should think how it fits your use case.
What about re-usability? could you re-use the keyboard as a component on the same page by calling only one function?

Best,
Martin

[zack1]

Without knowing how some of the other code is, you are clearly using ES6 in places so this could be a little more ES6-y.

{
  const keys = document.querySelectorAll('.key')
  
  document.addEventListener('keydown', playSound)
  document.addEventListener('transitionend', removeTransition)

  function playSound(e) {
    let audio = document.querySelector(`[audio-key="${e.keyCode}"]`),
         key = document.querySelector(`[data-key="${e.keyCode}"]`)
    if(!audio) return
    audio.currentTime = 0
    audio.play()
    key.classList.add('playing')
  }
  
  function removeTransition(e) {
    [].forEach.call(keys, (key) => {
      key.classList.remove('playing')
    })
  }
}

Some of the ideas here.
Instead of an immediately invoked function, and to keep the global scope clear, you can place all your code in a simple block.
{ ....code... }

And because you were immediately invoking the function, I assume you aren’t calling it by name, so I just got rid of audioController() altogether. All it really did is set up the two event listeners.

Set up the keys const outside of the functions and pass it in to any functions that needs it. If you only need this variable inside one single function, then define it inside that function with a let. Just depends how it’s used.

The ‘playSound’ function variables were changed to let instead of const, as they will be recreated/new variables each time a sound is played. The idea of a const is not necessary here.

And as Martin said, the loop in removeTransition is kind of weird. I’m not even sure how that works, since you’re calling forEach on an empty array and not really even using the passed in element object.

In any case, I don’t know if my code is functional but it’s a little “cleaned up” in the ES6 way.

Another thing to keep in mind is to try and create functions that receive their inputs from the calling code, rather than generating or creating state within the function, nor creating effects or changing variables outside it.

To do that means you’d probably have to create a handful of additional functions with smaller tasks, but using pure functions are more testable and reusable that way.

You could use variables instead of functions like const audioController() = () => { ... }. This alternative syntax may or may not have any benefit in terms of your entire app and how you code, and hoisting becomes a concern. But it can have certain benefits in some situations.

Anyway, good luck!

[n_merciano]

Thank you so much! That was exactly what i was hoping to get as a result… this way i can become a better programmer one day.

[Paul_Wilkins]

@zack1

function removeTransition(e) {
.forEach.call(keys, (key) => {
key.classList.remove(‘playing’)
})
}

There is a better idea for the forEach code there, and that is to use Array.from() to turn the element list in to an array.

function removeTransition(e) {
    Array.from(keys).forEach(key) => {
      key.classList.remove('playing')
    })
  }

[m3g4p0p]

@n_merciano

I would like to know if someone can tell me how i can simplify my code

Not exactly simpler, but in terms of efficiency it would be better not to query for those audio and key elements anew on each keypress, but just once initially and store them in a map (or actually just a plain object). For example like

const fillMap = dataProp => (map, current) => {
  const keyCode = current.dataset[dataProp]
  map[keyCode] = current
  return map
}

const audioEls = document.querySelectorAll(`[data-audio]`)
const keyEls = document.querySelectorAll(`[data-key]`)

const audioMap = Array
  .from(audioEls)
  .reduce(fillMap('audio'), {})

const keyMap = Array
  .from(keyEls)
  .reduce(fillMap('key'), {})

const playSound = ({ keyCode }) => {
  const audio = audioMap[keyCode]
  const key = keyMap[keyCode]

  if (!audio || !key) return

  audio.currentTime = 0
  audio.play()

  key.classList.add('playing')
}

document.addEventListener('keydown', playSound)

As for the class removal, you might not even need to iterate over all elements, but just remove the class from the event.target… depends on the effect you want to achieve though.

const removeClass = ({ target }) => {
  target.classList.remove('playing')
}

document.addEventListener('transitionend', removeClass)

Anyway, you might then wrap the initializing stuff in a dedicated function, and expose the relevant functions from a module:

const audioMap = {}
const keyMap = {}

const fillMap = (map, dataProp) => current => {
  const keyCode = current.dataset[dataProp]
  map[keyCode] = current
}

const playSound = ({ keyCode }) => {
  const audio = audioMap[keyCode]
  const key = keyMap[keyCode]

  if (!audio || !key) return

  audio.currentTime = 0
  audio.play()

  key.classList.add('playing')
}

const removeClass = ({ target }) => {
  target.classList.remove('playing')
}

const init = () => {
  const audioEls = document.querySelectorAll(`[data-audio]`)
  const keyEls = document.querySelectorAll(`[data-key]`)

  Array.from(audioEls).forEach(fillMap(audioMap, 'audio'))
  Array.from(keyEls).forEach(fillMap(keyMap, 'key'))

  document.addEventListener('keydown', playSound)
  document.addEventListener('transitionend', removeClass)
}

const audioController = {
  playSound,
  removeClass,
  init
}

export default audioController