4
\$\begingroup\$
  function playGreen(){
       var audio = document.getElementById("audioG");
       audio.play();
                 }
  function playRed(){
       var audio = document.getElementById("audioR");
       audio.play();
                 }
  function playYellow(){
       var audio = document.getElementById("audioY");
       audio.play();
                 }
  function playBlue(){
       var audio = document.getElementById("audioB");
       audio.play();
                 }

It will function as is.. but it seems like I should be able to reduce the four function's into one with a variable? can I get some guidance on that, along with any other pointers too.. just starting on the pseudo logic for the rest of the javascript.

  <div class="gameWrapper">
    <div class="backPlate">
      <div class="pushPad green" onclick="playGreen()" >
        <audio preload="auto"  id="audioG" data-pad="1">      <!-- upper left -->
          <source src="https://s3.amazonaws.com/freecodecamp/simonSound1.mp3" type="audio/mpeg"/>
        </audio>
      </div>

      <div class="pushPad red" onclick="playRed()" >
        <audio preload="auto"  id="audioR" data-pad="2">      <!-- upper right -->
          <source src="https://s3.amazonaws.com/freecodecamp/simonSound2.mp3" type="audio/mpeg"/>
        </audio>
      </div>

      <div class="pushPad yellow" onclick="playYellow()" >
        <audio preload="auto"  id="audioY" data-pad="3">      <!-- lower left -->
          <source src="https://s3.amazonaws.com/freecodecamp/simonSound3.mp3" type="audio/mpeg"/>
        </audio>
      </div>

      <div class="pushPad blue" onclick="playBlue()" >
        <audio preload="auto"  id="audioB" data-pad="4">      <!-- lower right -->
          <source src="https://s3.amazonaws.com/freecodecamp/simonSound4.mp3" type="audio/mpeg"/>
        </audio>
      </div>
      <div class="startCircle">

        <h3>Shall we<br>play a game?</h3>
      </div>
      <div class="centerCircle"></div>
    </div>
  </div>

https://codepen.io/TurtleWolf/pen/awdoxQ?editors=1010

\$\endgroup\$
  • \$\begingroup\$ Just noticed I have a dead zone where hover does not register.. ? scontent-iad3-1.xx.fbcdn.net/v/t31.0-8/… \$\endgroup\$ – TurtleWolf Jun 8 '17 at 22:00
  • 1
    \$\begingroup\$ quick fix: add pointer-events: none; to your .startCircle CSS. You might be interested in the <svg> element for more complex shapes. \$\endgroup\$ – le_m Jun 8 '17 at 22:16
6
\$\begingroup\$

Duplicate Code

Duplicate code is generally considered a bad practice since making a change to one version could require a change to all the versions. It's easier to maintain your code if design decisions are localized and close to what they do.

Fortunately, getting rid of the dulpicate code in your playCOLOR functions will be easy. You just need to note that the only differences between the four of them are the color and the id of the element.

Using an Object

One of the most concise ways to do this would be to use an object:

const audoByColor = {
  "red": document.getElementById("audioR"),
  "green": document.getElementById("audioG"),
  "blue": document.getElementById("audioB"),
  "yellow": document.getElementById("audioY")
};

That { ... }; thing is called an object, and the [color] part is requesting the value of the corresponding color key passed as a parameter. Objects are great at associating specific keys ("red", "green", ...) to specific values ("audoR", "audioG", ...).

Then in the blue element you could put:

<div class="pushPad blue" onclick="audoByColor['blue'].play()">

Using a switch statement

You can achieve a similar effect with a switch statement:

function getAudioByColor(color) {
  switch (color) {
    case "red":
      return document.getElementById("audioR");
    case "green":
      return document.getElementById("audioG");
    case "blue":
      return document.getElementById("audioB");
    case "yellow":
      return document.getElementById("audioY");
    default:
      return null;
  };
}

Note that switch statements are somewhat frowned upon since they can lead to scoping bugs and issues of forgetting to break or return. But they do come with the benefit that you can easily specify a default case.

Using if else

You could achieve the same effect as a switch statement in a safer way by using a list of if else blocks:

function getAudioByColor(color) {
  if (color === "red") {
    return document.getElementById("audioR");
  }
  else if (color === "green") {
    return document.getElementById("audioG");
  }
  else if (color === "blue") {
    return document.getElementById("audioB");
  }
  else if (color === "yellow") {
    return document.getElementById("audioY");
  }
  else {
    return null;
  }
}

I find this a bit cumbersome, but it might be the most obvious for someone reading your code.

\$\endgroup\$
  • 2
    \$\begingroup\$ function getAudioByColor(color) { return document.getElementById("audio" + ( (color || "r") [0] .toUpperCase() ) ); } is the most obvious definition imo, with no repetition \$\endgroup\$ – cat Jun 9 '17 at 0:25
  • \$\begingroup\$ @cat Clever, but not very readable. An explicit mapping of colors to selectors is easier to understand, modify, debug etc. \$\endgroup\$ – le_m Jun 9 '17 at 13:13
  • 2
    \$\begingroup\$ @le_m How? If it were on multiple lines, perhaps if we change the HTML IDs to be audio-green rather than audioG, it could be even cleaner / clearer \$\endgroup\$ – cat Jun 9 '17 at 14:46
  • 2
    \$\begingroup\$ @cat Just the classic 'stupidly simple but repetitive vs. condensed but requires thinking to understand' trade-off - I tend to go with the first one. I agree with your second suggestion to use audio-green instead of audioG. A better design would probably push the data-pad attributes from the audio elements up to the div containers and map container - audio based on that (and leverage the web audio api instead of hidden <audio>'s). \$\endgroup\$ – le_m Jun 9 '17 at 14:54
  • \$\begingroup\$ There's a lot to absorb here.. I've tried to read up on the Web API, all the tuturials I'm finding start with filtering the sound through a node first, I just want to play it each time a button's pushed, so recommendations on a good starting tutorial might get me a long way.. and perhaps I could have elaborated more in the original post. \$\endgroup\$ – TurtleWolf Jun 11 '17 at 16:34
6
\$\begingroup\$

I recommend removing the inline onclick event handlers from your HTML document for a cleaner separation of markup and logic / JavaScript similarly to how you separate markup and style / CSS.

You can then add all event handlers with a few lines of code as follows:

document.querySelectorAll('.pushPad').forEach(pad => {
  let audio = pad.querySelector('audio');
  pad.addEventListener('click', event => audio.play());
});

A better design would probably have the data-pad attributes on the colored buttons and not on hidden <audio> elements, introduce a mapping between pad and audio source and leverage the web audio API instead. Using <svg> shapes instead of heavily styled <div>s for the UI might further simplify your markup.

\$\endgroup\$
  • 2
    \$\begingroup\$ Why not getElementsByTagName for audio? \$\endgroup\$ – hjpotter92 Jun 9 '17 at 6:30
  • \$\begingroup\$ @hjpotter92 That might be a little bit faster. I chose querySelector for consistency with querySelectorAll \$\endgroup\$ – le_m Jun 9 '17 at 11:25
  • 2
    \$\begingroup\$ @hjpotter92 By the way, devdocs.io is really nice, thanks for that link \$\endgroup\$ – le_m Jun 9 '17 at 12:58
1
\$\begingroup\$

One option would be to do something like:

function play(node) {
  var audio = node.getElementsByTagName('audio')[0];
  audio.play();
}

<div class="pushPad green" onclick="play(this)" >
  <audio preload="auto"  id="audioG" data-pad="1">      <!-- upper left -->
    <source src="https://s3.amazonaws.com/freecodecamp/simonSound1.mp3" type="audio/mpeg"/>
  </audio>
</div>

However I don't really like nesting hidden audio elements like this, I would put the audio nodes together at the bottom of the document and do something like:

function play(node) {
  var audioId = node.attributes['data-audio-id'].value;
  console.log('audioId', audioId);
  var audio = document.getElementById(audioId);
  audio.play();
}

<div class="pushPad green" onclick="play(this)" ></div>

... At the bottom of my document.
<audio preload="auto"  id="audioG" data-pad="1">      <!-- upper left -->
  <source src="https://s3.amazonaws.com/freecodecamp/simonSound1.mp3" type="audio/mpeg"/>
</audio>
\$\endgroup\$

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.