Should all declarations be kept at the top?

What I’m trying to find out is,
is it better to group the selectors together, or not.

Some people say they should be defined where they are used,
other people say to keep them at the top.

Is there a consensus on this, or is this more of a preference thing?

Does keeping the selectors that match the handlers make more sense?

Like how this one is set up?

(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;
    });
}());

or is it better to group them altogether like how this one is set up?

(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.src = value.value;
        player.volume = 1.0;

    });
}());

There are different varying thoughts and opinions on this:

Declarations on Top

It is a good coding practice to put all declarations at the top of each script or function.

This will:

Give cleaner code
Provide a single place to look for local variables
Make it easier to avoid unwanted (implied) global variables
Reduce the possibility of unwanted re-declarations

w3schools
https://www.w3schools.com/js/js_best_practices.asp

When I set up guidelines for a new C project I always state that it is better to declare the variables close to where they are used. For two reasons, it makes it easier to refactor the code later on (i.e. when extracting a method). It also helps the compiler to do better optimization.

https://softwareengineering.stackexchange.com/questions/119205/what-is-the-possible-disadvantage-of-putting-declarations-in-inner-blocks-inste

Variables should be declared as locally as possible.

Declaring variables “at the top of the function” is always a disastrously bad practice. Even in C89/90 language, where variables can only be declared at the beginning of the block, it is better to declare them as locally as possible, i.e. at the beginning of smallest local block that covers the desired lifetime of the variable. Sometimes it might even make sense to introduce a “redundant” local block with the only purpose of “localizing” the variable declaration.

https://stackoverflow.com/a/3773458

Should I declare variables at the top of the function for reasons other than the scope rules?

Each computer language has its strengths and pitfalls. Each has its own coding standards/practices often accepted, built and dictated by the community around it. Because programming is a precise art I would say, unless there is a good reason to do something, you shouldn’t.

Now there are very good reasons to declare variables close to where they are used:

Makes it easy to determine the type, when reading code.
Makes it easy to delete code including the declaration all together.

https://softwareengineering.stackexchange.com/a/300034

Your first sub-para suggests there is no consensus. I would suggest that there is a degree of personal preference in this. I usually declare variables (including those encapsulating selectors) at the top of what I do.

Other than that, if you are using use strict, then they need to be declared before you try to use them.

2 Likes

Which would mean leaving them at the top like how this one is:

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

IMHO the main thing is to be consistent. Unless you are writing code as part of a team or for something that has a certain writing style as a requirement it is up to your preference. Find what works for you and stick to it.

5 Likes

Personally I’m an advocate of maintaining all the variables together at the top of the functions, although with const / let and strict mode that probably isn’t as important any more as it used to be. That said, if the question even arises this might be an indication that it’s time to refactor your code into multiple functions:

const initPlayButton = () => {
  const player = document.getElementById('player')
  const button = document.getElementById('button')

  button.addEventListener('click', () => {
    if (player.paused) {
      player.play()
    } else {
      player.pause()
    }
  })
}

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

initPlayButton()
initSentButton()
5 Likes

Both examples were using C++ which is a compiled language and very different from JavaScript which is an interpreted language. It is like comparing apples to oranges. Wiki has detailed explanations which is worth reading.

My knowledge of Javascriot functions is limited and glancing at the examples would imagine the function flexibility would be improved by passing four parameters instead of declaring constants inside the function.

I think functions should never have hard-coded inside because the function is severely restricted, Parameters passed to the same function would then be ideal for some sort of library because it could be used multiple times.

How would I be able to add that to this?

I tried to refactor, but it doesn’t seem to be working in this code, or I haven’t been able to figure it out.

Working Code:

I improved it from the last previous working code.

In this code clicking ‘set’ triggers the play button to change.

And to pause it, you have to click on the button itself.

(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', () => {
        if (player.paused) {
            player.play();
            playPauseIcon(true);
        } else {
            player.pause();
            playPauseIcon(false);
        }
    });
    sent.addEventListener('click', () => {
        player.volume = 1.0;
        player.src = value.value;
        playPauseIcon(true);
    });
}());

How would I add this in?

const initPlayButton = () => {
  const player = document.getElementById('player')
  const button = document.getElementById('button')


const initSentButton = () => {
  const value = document.getElementById('input')
  const sent = document.getElementById('sent')

initPlayButton()
initSentButton()

This is what I had in mind:


...
...
function life ( 
player = document.getElementById('player'),
button = document.getElementById('button'),
input  = document.getElementById('input'),
sent = document.getElementById('sent')
)

Edit:
In other languages if no parameters are passed the player, button, input and sent would use the default parameters otherwise use the supplied parameters.

One of the best reasons to enforce declaring variables at the top of functions, is that when “too many” variables are being declared, that’s a good clear sign that the function should be split up into different pieces.

If you don’t declare them at the top of the function and instead sprinkle them throughout the code, it becomes much more difficult to notice when your code can be beneficially improved.

5 Likes

Should:
let hasShuffled = false;

Be moved underneath:?
const players = [];

Code:

const videoPlayer = (function makeVideoPlayer() {
  "use strict";
  const players = [];

  function onPlayerReady(event) {
    const player = event.target;
    player.setVolume(100); // percent
  }

  let hasShuffled = false;

  function onPlayerStateChange(event) {
    const player = event.target;
    if (!hasShuffled) {
      player.setShuffle(true);
      player.playVideoAt(0);
      hasShuffled = true;
    }

Yes.

1 Like

Where would this const go, would it stay where it is, or would I move it up, underneath a function?

I think it would stay where it is because if I try placing it in any other location, the code breaks and is unusable.

const playlist = "Nbp8XZnzfT8,mnfmQe8Mv1g,-Xgi_way56U,CHahce95B1g";

loadPlayer({
  target: ".jacketc",
  width: 600,
  height: 338,
  loop: true,
  playlist
});

Code:

const load = (function makeLoad() {
  "use strict";

  function _load(tag) {
    return function(url) {
      return new Promise(function(resolve) {
        const element = document.createElement(tag);
        const parent = "body";
        const attr = "src";
        element.onload = function() {
          resolve(url);
        };
        element[attr] = url;
        document[parent].appendChild(element);
      });
    };
  }
  return {
    js: _load("script")
  };
}());

(function manageCover() {
  "use strict";
  const hide = (el) => el.classList.add("hide");

  function coverClickHandler(evt) {
    const cover = evt.currentTarget;
    hide(cover);
  }
  const cover = document.querySelector(".jacketc");
  cover.addEventListener("click", coverClickHandler);
}());
(function manageCover() {
  "use strict";
  const show = (el) => el.classList.remove("hide");
  const hide = (el) => el.classList.add("hide");

  function coverClickHandler(evt) {
    const cover = evt.currentTarget;
    const thewrap = cover.parentNode.querySelector(".wraph");
    hide(cover);
    show(thewrap);
  }
  const cover = document.querySelector(".jacketd ");
  cover.addEventListener("click", coverClickHandler);
}());
const videoPlayer = (function makeVideoPlayer() {
  "use strict";
  const players = [];
  let hasShuffled = false;

  function onPlayerReady(event) {
    const player = event.target;
    player.setVolume(100); // percent
  }

  function onPlayerStateChange(event) {
    const player = event.target;
    if (!hasShuffled) {
      player.setShuffle(true);
      player.playVideoAt(0);
      hasShuffled = true;
    }
    if (event.data === YT.PlayerState.PLAYING) {
      const otherVideos = (video) => video !== player;
      const pauseVideo = (video) => video.pauseVideo();
      players.filter(otherVideos).forEach(pauseVideo);
    }

    const playerVars = player.b.b.playerVars;
    if (playerVars.loop && event.data === YT.PlayerState.ENDED) {
      player.seekTo(playerVars.start);
    }
  }

  function addVideo(video, settings) {
    players.push(new YT.Player(video, Object.assign({
      videoId: video.dataset.id,
      host: "https://www.youtube-nocookie.com",
      events: {
        "onReady": onPlayerReady,
        "onStateChange": onPlayerStateChange
      }
    }, settings)));
  }

  function init(video, settings) {
    load.js("https://www.youtube.com/player_api").then(function() {
      YT.ready(function() {
        addVideo(video, settings);
      });
    });
  }
  return {
    init
  };
}());

function loadPlayer(opts) {
  "use strict";
  const show = (el) => el.classList.remove("hide");

  function initPlayer(wrapper) {
    const video = wrapper.querySelector(".video");
    opts.width = opts.width || 198;
    opts.height = opts.height || 198;
    opts.autoplay = 1;
    opts.controls = 1;
    opts.rel = 0;
    opts.iv_load_policy = 3;
    opts.fs = 0;
    opts.disablekb = 1;

    function paramInOpts(settings, param) {
      if (opts[param] !== undefined) {
        settings[param] = opts[param];
      }
      return settings;
    }
    const settingsParams = ["width", "height", "videoid", "host"];
    const settings = settingsParams.reduce(paramInOpts, {});
    const playerVarsParams = ["autoplay", "cc_load_policy",
      "controls", "disablekb", "end", "fs", "iv_load_policy",
      "list", "listType", "loop", "playlist", "rel", "start"
    ];
    settings.playerVars = playerVarsParams.reduce(paramInOpts, {});
    videoPlayer.init(video, settings);
  }

  function coverClickHandler(evt) {
    const wrapper = evt.currentTarget.nextElementSibling;
    show(wrapper);
    initPlayer(wrapper);
  }
  const cover = document.querySelector(opts.target);
  cover.addEventListener("click", coverClickHandler);
}
const playlist = "Nbp8XZnzfT8,mnfmQe8Mv1g,-Xgi_way56U,CHahce95B1g";

loadPlayer({
  target: ".jacketc",
  width: 600,
  height: 338,
  loop: true,
  playlist
});
loadPlayer({
  target: ".alpha",
  start: 0,
  end: 280,
  loop: true
});
loadPlayer({
  target: ".beta",
  start: 0,
  end: 240,
  loop: true
});
loadPlayer({
  target: ".gamma",
  start: 0,
  end: 222,
  loop: true
});
loadPlayer({
  target: ".delta",
  start: 4,
  end: 254,
  loop: true
});
loadPlayer({
  target: ".epsilon",
  start: 0,
  end: 150,
  loop: true
});
loadPlayer({
  target: ".zeta",
  start: 0,
  end: 285,
  loop: true
});
loadPlayer({
  target: ".eta",
  start: 23,
  end: 312,
  loop: true
});
loadPlayer({
  target: ".theta",
  start: 12
});
loadPlayer({
  target: ".iota"
});

The general pattern is variable declarations first, then the functions, and then other code.

However, that playlist one can be “inlined” instead. It’s used only the once, and doesn’t benefit from being defined separately. It can be moved instead it to the object where it is used.

// const playlist = "Nbp8XZnzfT8,mnfmQe8Mv1g,-Xgi_way56U,CHahce95B1g";
loadPlayer({
  target: ".jacketc",
  width: 600,
  height: 338,
  loop: true,
  // playlist
  playlist: "Nbp8XZnzfT8,mnfmQe8Mv1g,-Xgi_way56U,CHahce95B1g"
});
1 Like

The only bad thing about doing it that way is if I wanted to add more songs on to it.

This isn’t allowed:

playlist:"Nbp8XZnzfT8,mnfmQe8Mv1g,Xgi_way56U,
CHahce95B1g,CHahce95B1g,CHahce95B1g,CHahce95B1g"
});

Not a problem, you can use the + symbol to join two strings together, even if they are on separate lines.

1 Like

I got it:

loadPlayer({
    target: ".jacketc",
    width: 600,
    height: 338,
    loop: true,
    playlist: "Nbp8XZnzfT8,mnfmQe8Mv1g,-Xgi_way56U,CHahce95B1g" +
        ",u8hvzxYSPjQ,PEJpJ1UKQpg,pIuWymsUuk0,qYEooPeyz5M,qYEooPeyz5M"
});

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