Which Javascript code do you like better?


#1

Both are set up differently.

Code 1
https://jsfiddle.net/pmnfxz4v/23/

<script>
(function iife() {
    "use strict";

    function playButtonClickHandler() {
        var button = document.querySelector(".playButton2");
        var player = document.querySelector("#player2");
        document.querySelector(".playButton2 .initial2").style.display = "none";
        player.volume = 1.0;
        if (player.paused) {
            button.classList.add("playing");
            document.querySelector(".playButton2 .play2").style.display = "none";
            document.querySelector(".playButton2 .pause2").style.display = "inline-block";
            player.play();
        } else {
            document.querySelector(".playButton2 .play2").style.display = "inline-block";
            document.querySelector(".playButton2 .pause2").style.display = "none";
            player.pause();
        }
    }

    function playButtonMouseoverHandler() {
        var player = document.querySelector("#player2");
        player.isPlaying = function isPaused() {
            return player.paused === false;
        };
        if (player.isPlaying()) {
            document.querySelector(".playButton2 .speaker2").style.display = "none";
            document.querySelector(".playButton2 .pause2").style.display = "inline-block";
        }
    }

    function playButtonMouseoutHandler() {
        var player = document.querySelector("#player2");
        player.isPlaying = function isPlaying() {
            return player.paused === false;
        };
        if (player.isPlaying()) {
            document.querySelector(".playButton2 .pause2").style.display = "none";
            document.querySelector(".playButton2 .speaker2").style.display = "inline-block";
        }
    }
    var playButton = document.querySelector(".playButton2");
    playButton.addEventListener("click", playButtonClickHandler);
    playButton.addEventListener("mouseover", playButtonMouseoverHandler);
    playButton.addEventListener("mouseout", playButtonMouseoutHandler);
}());
</script>

Code 2
https://jsfiddle.net/fsesrh2b/1/

<script>
var utils = (function () {
    "use strict";
    function show(el) {
        el.classList.remove("hide");
    }
    function hide(el) {
        el.classList.add("hide");
    }
    function hideAll(items) {
        items.forEach(function hideItem(item) {
            hide(item);
        });
    }

    return {
        show,
        hide,
        hideAll
    };
}());
var images = (function (utils) {
    "use strict";
    var icons = new Map();

    function getSVGIcons(button) {
        var buttonSVG = button.querySelectorAll("svg");
        Array.from(buttonSVG).forEach(function addIcon(svg) {
            var name = svg.getAttribute("class").split(" ")[0];
            icons.set(name, svg);
        });
        return icons;
    }
    function getSVGIcon(name) {
        return icons.get(name);
    }
    function showIcon(iconName) {
        utils.hideAll(icons);
        utils.show(getSVGIcon(iconName));
    }

    return {
        getSVGIcons,
        getSVGIcon,
        showIcon
    };
}(utils));
var togglePlayer = (function (images) {
    "use strict";
    var button;
    var player;

    function isPlaying() {
        return player.paused === false;
    }
    function toggle() {
        if (isPlaying()) {
            player.pause();
            images.showIcon("play");
        } else {
            player.play();
            images.showIcon("pause");
        }
    }
    function togglePlayingHandler() {
        button.classList.add("triggered");
        toggle();
    }
    function showPauseHandler() {
        if (isPlaying()) {
            images.showIcon("pause");
        }
    }
    function showSpeakerHandler() {
        if (isPlaying()) {
            images.showIcon("speaker");
        }
    }
    function init(playButton, audioPlayer) {
        button = playButton;
        player = audioPlayer;
        images.getSVGIcons(button);

        player.volume = 1.0;

        button.addEventListener("click", togglePlayingHandler, false);
        button.addEventListener("mouseover", showPauseHandler, false);
        button.addEventListener("mouseout", showSpeakerHandler, false);
    }
    return {
        init
    };
}(images));
var init = (function (togglePlayer) {
    "use strict";
    function iife() {
        var button = document.querySelector(".playButton");
        var player = document.querySelector(".player");
        togglePlayer.init(button, player);
    }
    iife();
}(togglePlayer));

  </script>

#2

Code 3
https://jsfiddle.net/muek65ss/18/

<style>
  .playButton3 {
    display: inline-block;
    cursor: pointer;
    height: 44px;
    background-color: black;
    border-top: 3px solid #0059dd;
    border-bottom: 3px solid #0059dd;
  }
  
  .pause3 {
    display: none;
  }
  
  .playButton3.svoefm {
    width: 83px;
    border-left: 3px solid #0059dd;
    fill: #aaff00;
  }
  
  .playButton3.soundpark {
    width: 88px;
    border-left: 3px solid #0059dd;
    border-right: 3px solid #0059dd;
    fill: #ffaa00;
  }
  
  .playButton3.cavoparadisoclub {
    width: 83px;
    border-right: 3px solid #0059dd;
    fill: #ff00aa;
  }
  
  .playButtons {
    white-space: nowrap;
    font-size: 0;
  }

</style>

<div class="playButtons">
  <div class="playButton3 svoefm">
    <svg class="play3" width="12" height="14" style="margin:15px 36px;" viewbox="0 0 85 100">
      <path d="M81 44.6c5 3 5 7.8 0 10.8L9 98.7c-5 3-9 .7-9-5V6.3c0-5.7 4-8 9-5l72 43.3z"></path>
    </svg>

    <svg class="pause3" width="10" height="14" style="margin:15px 37px;" viewbox="0 0 60 100" >
      <path d="M0 8c0-5 3-8 8-8s9 3 9 8v84c0 5-4 8-9 8s-8-3-8-8V8zm43 0c0-5 3-8 8-8s8 3 8 8v84c0 5-3 8-8 8s-8-3-8-8V8z"></path>
    </svg>

    <audio preload="none">
      <source src="http://getradio.me/svoefm" type="audio/mpeg">
    </source></audio>
  </div>

  <div class="playButton3 soundpark">
    <svg class="play3" width="12" height="14" style="margin:15px 39px;" viewbox="0 0 85 100">
      <path d="M81 44.6c5 3 5 7.8 0 10.8L9 98.7c-5 3-9 .7-9-5V6.3c0-5.7 4-8 9-5l72 43.3z"></path>
    </svg>

    <svg class="pause3" width="10" height="14" style="margin:15px 40px;" viewbox="0 0 60 100">
      <path d="M0 8c0-5 3-8 8-8s9 3 9 8v84c0 5-4 8-9 8s-8-3-8-8V8zm43 0c0-5 3-8 8-8s8 3 8 8v84c0 5-3 8-8 8s-8-3-8-8V8z"></path>
    </svg>

    <audio preload="none">
      <source src="http://getradio.me/spdeep" type="audio/mpeg">
    </source></audio>
  </div>

  <div class="playButton3 cavoparadisoclub">
    <svg class="play3" width="12" height="14" style="margin:15px 36px;" viewbox="0 0 85 100" >
      <path d="M81 44.6c5 3 5 7.8 0 10.8L9 98.7c-5 3-9 .7-9-5V6.3c0-5.7 4-8 9-5l72 43.3z"></path>
    </svg>

    <svg class="pause3" width="10" height="14" style="margin:15px 37px;" viewbox="0 0 60 100">
      <path d="M0 8c0-5 3-8 8-8s9 3 9 8v84c0 5-4 8-9 8s-8-3-8-8V8zm43 0c0-5 3-8 8-8s8 3 8 8v84c0 5-3 8-8 8s-8-3-8-8V8z"></path>
    </svg>

    <audio preload="none">
      <source src="http://s5.onweb.gr:8488/;">
    </source></audio>
  </div>
</div>

<script>

(function iife() {
    "use strict";

    function getButton(el) {
        while (el.nodeName !== "HTML" && el.nodeName !== "DIV") {
            el = el.parentNode;
        }
        return el;
    }

    function playButton3(event) {
        var button = getButton(event.target);
        var player = button.querySelector("audio");
        var play = button.querySelector(".play3");
        var pause = button.querySelector(".pause3");
        button.classList.add("clicked");
        player.volume = 1.0;

        if (player.paused) {
            play.style.display = "none";
            pause.style.display = "inline-block";
            player.play();
        } else {
            play.style.display = "inline-block";
            pause.style.display = "none";
            player.pause();
        }
    }


    var playButtons = document.querySelectorAll(".playButton3");
    playButtons.forEach(function addPlayButtonHandler(el) {
        el.addEventListener("click", playButton3);
    });
}());


</script>

Code 4
https://jsfiddle.net/muek65ss/17/

<style>
  .playButtons,
  .playButtons div {
    display: inline-block;
  }
  
  .playButtons {
    background-color: #000000;
    border-top: 3px solid #0059dd;
    border-bottom: 3px solid #0059dd;
    cursor: pointer;
    height: 44px;
    white-space: nowrap;
    font-size: 0;
  }
  
  .playButton:nth-child(1) {
    width: 83px;
    border-left: 3px solid #0059dd;
    fill: #aaff00;
  }
  
  .playButton:nth-child(2) {
    width: 88px;
    border-left: 3px solid #0059dd;
    border-right: 3px solid #0059dd;
    fill: #ffaa00;
  }
  
  .playButton:nth-child(3) {
    width: 83px;
    border-right: 3px solid #0059dd;
    fill: #ff00aa;
  }
  
  .hide {
    display: none;
  }

</style>

<div class="playButtons">

  <div class="playButton">
    <svg class="play" style="margin:15px 36px;" width="12" height="14" viewbox="0 0 85 100">
      <path d="M81 44.6c5 3 5 7.8 0 10.8L9 98.7c-5 3-9 .7-9-5V6.3c0-5.7 4-8 9-5l72 43.3z"></path>
    </svg>
    <svg class="pause hide" style="margin:15px 37px;" width="10" height="14" viewbox="0 0 60 100">
      <path d="M0 8c0-5 3-8 8-8s9 3 9 8v84c0 5-4 8-9 8s-8-3-8-8V8zm43 0c0-5 3-8 8-8s8 3 8 8v84c0 5-3 8-8 8s-8-3-8-8V8z"></path>
    </svg>

    <audio preload="none">
      <source src="http://hi5.1980s.fm/" type="audio/mpeg">
    </audio>
  </div>

  <div class="playButton">

    <svg class="play" style="margin:15px 39px;" width="12" height="14" viewbox="0 0 85 100">
      <path d="M81 44.6c5 3 5 7.8 0 10.8L9 98.7c-5 3-9 .7-9-5V6.3c0-5.7 4-8 9-5l72 43.3z"></path>
    </svg>

    <svg class="pause hide" style="margin:15px 40px;" width="10" height="14" viewbox="0 0 60 100">
      <path d="M0 8c0-5 3-8 8-8s9 3 9 8v84c0 5-4 8-9 8s-8-3-8-8V8zm43 0c0-5 3-8 8-8s8 3 8 8v84c0 5-3 8-8 8s-8-3-8-8V8z"></path>
    </svg>

    <audio preload="none">
      <source src="http://hi5.1980s.fm/" type="audio/mpeg">
    </audio>
  </div>

  <div class="playButton">

    <svg class="play" style="margin:15px 36px;" width="12" height="14" viewbox="0 0 85 100">
      <path d="M81 44.6c5 3 5 7.8 0 10.8L9 98.7c-5 3-9 .7-9-5V6.3c0-5.7 4-8 9-5l72 43.3z"></path>
    </svg>

    <svg class="pause hide" style="margin:15px 37px;" width="10" height="14" viewbox="0 0 60 100">
      <path d="M0 8c0-5 3-8 8-8s9 3 9 8v84c0 5-4 8-9 8s-8-3-8-8V8zm43 0c0-5 3-8 8-8s8 3 8 8v84c0 5-3 8-8 8s-8-3-8-8V8z"></path>
    </svg>
    <audio preload="none">
      <source src="http://hi5.1980s.fm/;" type="audio/mpeg">
    </audio>
  </div>
</div>

<script>
  (function iife() {
    "use strict";

    function togglePlayer(button) {
      var player = button.querySelector("audio");
      player.volume = 1.0;
      if (player.paused) {
        button.querySelector(".pause").classList.remove("hide");
        button.querySelector(".play ").classList.add("hide");
        player.play();
      } else {
        button.querySelector(".pause").classList.add("hide");
        button.querySelector(".play ").classList.remove("hide");
        player.pause();
      }
    }

    function upTo(el, className) {
      while (el !== "html" && !el.classList.contains(className)) {
        el = el.parentNode;
      }
      return el;
    }

    function playButtonClickHandler(evt) {
      var playButton = upTo(evt.target, "playButton");
      togglePlayer(playButton);
    }
    var playButtons = document.querySelectorAll(".playButtons");
    playButtons.forEach(function addHandler(playButton) {
      playButton.addEventListener("click", playButtonClickHandler);
    });
  }());

</script>

#3

Code 1 has many assumptions that results in lots of rewriting of the code when things change.
Code 2 has less assumptions and is more flexible.

Therefore, Code 2 is the preferred code.


#4

Code 3 relies too heavily on class names to retrieve a reference to elements. Ideally only the one class name should be used, when other elements are easily obtained from a reference to the main one.

Code 4 make many assumptions that must be rewritten when changes occur, and is the worst code of the two.

Therefore Code 3 is preferred, but could be a lot better.


#5

I improved code 3 somewhat here, by using position absolute, instead of margin, but not sure how much of an improvement that is, or if it's even an improvement at all.

But you say it can be improved even more, let's do that.

https://jsfiddle.net/95ejqse4/3/

<style>
  .playButton3 {
    position: relative;
    display: inline-block;
    cursor: pointer;
    height: 44px;
    background-color: black;
    border-top: 3px solid #0059dd;
    border-bottom: 3px solid #0059dd;
  }
  
  .play3 {
    position: absolute;
    left: 0;
    top: 0;
    right: 0;
    bottom: 0;
    margin: auto;
  }
  
  .pause3 {
    position: absolute;
    left: 0;
    top: 0;
    right: 0;
    bottom: 0;
    margin: auto;
    display: none;
  }
  
  .playButton3.svoefm {
    width: 83px;
    border-left: 3px solid #0059dd;
    fill: #aaff00;
  }
  
  .playButton3.soundpark {
    width: 88px;
    border-left: 3px solid #0059dd;
    border-right: 3px solid #0059dd;
    fill: #ffaa00;
  }
  
  .playButton3.cavoparadisoclub {
    width: 83px;
    border-right: 3px solid #0059dd;
    fill: #ff00aa;
  }
  
  .playButtons {
    white-space: nowrap;
    font-size: 0;
  }

</style>

<div class="playButtons">
  <div class="playButton3 svoefm">
    <svg class="play3" width="12" height="14" viewbox="0 0 85 100">
      <path d="M81 44.6c5 3 5 7.8 0 10.8L9 98.7c-5 3-9 .7-9-5V6.3c0-5.7 4-8 9-5l72 43.3z"></path>
    </svg>

    <svg class="pause3" width="10" height="14" viewbox="0 0 60 100">
      <path d="M0 8c0-5 3-8 8-8s9 3 9 8v84c0 5-4 8-9 8s-8-3-8-8V8zm43 0c0-5 3-8 8-8s8 3 8 8v84c0 5-3 8-8 8s-8-3-8-8V8z"></path>
    </svg>

    <audio preload="none">
      <source src="http://getradio.me/svoefm" type="audio/mpeg">
      </source>
    </audio>
  </div>

  <div class="playButton3 soundpark">
    <svg class="play3" width="12" height="14" viewbox="0 0 85 100">
      <path d="M81 44.6c5 3 5 7.8 0 10.8L9 98.7c-5 3-9 .7-9-5V6.3c0-5.7 4-8 9-5l72 43.3z"></path>
    </svg>

    <svg class="pause3" width="10" height="14" viewbox="0 0 60 100">
      <path d="M0 8c0-5 3-8 8-8s9 3 9 8v84c0 5-4 8-9 8s-8-3-8-8V8zm43 0c0-5 3-8 8-8s8 3 8 8v84c0 5-3 8-8 8s-8-3-8-8V8z"></path>
    </svg>

    <audio preload="none">
      <source src="http://getradio.me/spdeep" type="audio/mpeg">
      </source>
    </audio>
  </div>

  <div class="playButton3 cavoparadisoclub">
    <svg class="play3" width="12" height="14" viewbox="0 0 85 100">
      <path d="M81 44.6c5 3 5 7.8 0 10.8L9 98.7c-5 3-9 .7-9-5V6.3c0-5.7 4-8 9-5l72 43.3z"></path>
    </svg>

    <svg class="pause3" width="10" height="14" viewbox="0 0 60 100">
      <path d="M0 8c0-5 3-8 8-8s9 3 9 8v84c0 5-4 8-9 8s-8-3-8-8V8zm43 0c0-5 3-8 8-8s8 3 8 8v84c0 5-3 8-8 8s-8-3-8-8V8z"></path>
    </svg>

    <audio preload="none">
      <source src="http://s5.onweb.gr:8488/;">
      </source>
    </audio>
  </div>
</div>

<script>
  (function iife() {
    "use strict";

    function getButton(el) {
      while (el.nodeName !== "HTML" && el.nodeName !== "DIV") {
        el = el.parentNode;
      }
      return el;
    }

    function playButton3(event) {
      var button = getButton(event.target);
      var player = button.querySelector("audio");
      var play = button.querySelector(".play3");
      var pause = button.querySelector(".pause3");
      button.classList.add("clicked");
      player.volume = 1.0;

      if (player.paused) {
        play.style.display = "none";
        pause.style.display = "inline-block";
        player.play();
      } else {
        play.style.display = "inline-block";
        pause.style.display = "none";
        player.pause();
      }
    }


    var playButtons = document.querySelectorAll(".playButton3");
    playButtons.forEach(function addPlayButtonHandler(el) {
      el.addEventListener("click", playButton3);
    });
  }());

</script>

#6

I just reduced it even more by combining both
.play3,.pause3
position: absolute.
https://jsfiddle.net/hrLec13q/

 .play3,.pause3 {
    position: absolute;
    left: 0;
    top: 0;
    right: 0;
    bottom: 0;
    margin: auto;
  }
  
  .pause3 {
    display: none;
  }

#7

I just made another slight adjustment.
https://jsfiddle.net/h1crm6st/

Placing
width: 83px;

With:

.playButton3{
width: 83px;

And removing it from these

.playButton3.svoefm {
width: 83px;

.playButton3.cavoparadisoclub {
width: 83px;

#8

Even though code 1 javascript character count is: 1874
https://jsfiddle.net/2amdL4os/2/

Code 2 is: 2346
https://jsfiddle.net/2amdL4os/1/

Are you saying character count doesn't matter?


#9

What do you mean by that?


#10

If you want to care about character count, go and play code golf. It doesn't help to speed things up though, and when you need to make changes to it you can't.

Caring about character count especially at your stage of development, is a strong warning that you are inappropriately focusing too much on early optimization.

After all, it is written in stone:


#11

When the "playButton2" element is renamed to something else, how many parts of the JavaScript needs to change?
10 different parts of the scripting code need to change. If you made a mistake when making all of those changes then you aren't likely to notice that mistake until it's too late.

When it comes to referring to elements, you shouldn't fetch a reference to an element more than once.

Among other things that I notice in the scripting code, is that style.display is being used to change the CSS, which is reaching far too much in to the realm of CSS to make changes. Instead of doing that, just modify the class name to allow CSS to do it's job in a much more efficient manner.


#12

Whoops, I forgot to add the link. It was a long holiday and my pillow is looking extremely attractive right now.

Here's that code golf link, updated in the above post too.


#13

This topic was automatically closed 91 days after the last reply. New replies are no longer allowed.