Which of these codes is better set up?

or is there a better way to structure the code?

Code 1
https://jsfiddle.net/192h0w85/71/

<audio autoplay id="player"></audio>

<button class="style" onclick="togglePlay()">Play / Pause</button>

<script>
  function togglePlay() {
    const player = document.getElementById('player');
    if (player.paused) {
      player.play();
    } else {
      player.pause();
    }
  }

</script>

<script>
  document.getElementById('sent').addEventListener('click', function() {
    const player = document.getElementById('player');
    const value = document.getElementById('input');
    player.volume = 1.0;
    player.src = value.value;
  });
</script>

Code 2
https://jsfiddle.net/192h0w85/45/

<audio autoplay id="player"></audio>

<button class="style" onclick="document.getElementById('player');player.paused ? player.play() : player.pause()">Play / Pause</button>

<script>
  document.getElementById('sent').addEventListener('click', function() {
    const player = document.getElementById('player');
    const value = document.getElementById('input');
    player.volume = 1.0;
    player.src = value.value;
  });

</script>

The first one, but avoid inline handlers as a rule (get the element in the JS and add a clock handler to play/pause, don’t put it in the HTML).

Also cache your selectors, for example with this:

document.getElementById('sent').addEventListener('click', function() {
    const player = document.getElementById('player');
    const value = document.getElementById('input');
    player.volume = 1.0;
    player.src = value.value;
  });

You’re looking up the elements every time the click event fires. If instead you do

const player = document.getElementById('player');
const value = document.getElementById('input');

document.getElementById('sent').addEventListener('click', function() {
    player.volume = 1.0;
    player.src = value.value;
  });

By moving them out of the callback, they only get looked up once.

1 Like

Did I do this correctly?

If I did, can I make any improvements to it?

My Attempt:
https://jsfiddle.net/192h0w85/110/

(function iife() {
    "use strict";
    const player = document.getElementById("player");
    const button = document.getElementById("button");
    button.addEventListener("click", function () {
        if (player.paused) {
            player.play();
        } else {
            player.pause();
        }
    });
}());
(function iife() {
    "use strict";
    const player = document.getElementById("player");
    const value = document.getElementById("input");
    document.getElementById("sent").addEventListener("click", function () {
        player.volume = 1.0;
        player.src = value.value;
    });
}());

That looks fine for starters. I’d just have one script though, not sure the iife is really necessary either but it definitely doesn’t hurt to keep things contained

(function iife() {
    "use strict";

    const player = document.getElementById("player");
    const button = document.getElementById("button");
    const value = document.getElementById("input");
    const sent = document.getElementById("sent");

    button.addEventListener("click", function () {
        if (player.paused) {
            player.play();
        } else {
            player.pause();
        }
    });
    
    sent.addEventListener("click", function () {
        player.volume = 1.0;
        player.src = value.value;
    });
}());
1 Like

Does const always go at the top, you wouldn’t split them like this?

(function iife() {
    "use strict";
    const player = document.getElementById("player");
    const button = document.getElementById("button");
   
    button.addEventListener("click", function () {
        if (player.paused) {
            player.play();
        } else {
            player.pause();
        }
    }); 
    
    const value = document.getElementById("input");
    const sent = document.getElementById("sent");
    sent.addEventListener("click", function () {
        player.volume = 1.0;
        player.src = value.value;
    });
}());

No, not at all, it’s fine to have it like that: I just grouped all of the selectors then both of the event handlers, but keeping the selectors that match the handlers together makes just as much sense.

2 Likes

FYI. const is not intrinsically linked to document.getelementById("element");
const is merely a way to declare a variable just like let, and the obsolete var. So:

let myElement = document.getElementById("element");

is perfectly legal code (as would be var)

const is used when you want to assign a value to your variable and you do not want to risk it being overwritten (like your element selectors here). let is used when you are expecting the variable’s value to change. Like let i = 0; inside a for loop. If you used const here it would throw an error the first time it tried to increment i. var is used when you are following code examples that were written before let and const existed or working on brownfield projects.

My personal rule is “Always use const unless you have to use let and never use var.”

2 Likes

Also its best practise to keep all declerations in the top, to keep things organised.

2 Likes

I further improved the code here:

Now when set is clicked, the svg changes also.
https://jsfiddle.net/9krezgua/1/

(function iife() {
    "use strict";
    const player = document.getElementById("player");
    const button = document.getElementById("button");
    const value = document.getElementById("input");
    const sent = document.getElementById("sent");

    function playPauseIcon(play) {
        if (play) {
            button.classList.add("is-playing");
        } else {
            button.classList.remove("is-playing");
        }
    }
    button.addEventListener("click", function () {
        if (player.paused) {
            player.play();
            playPauseIcon(true);
        } else {
            player.pause();
            playPauseIcon(false);
        }
    });
    sent.addEventListener("click", function () {
        player.volume = 1.0;
        player.src = value.value;
        playPauseIcon(true);
    });
}());