3
\$\begingroup\$

I'm writing a simple start page and it's my first time with JavaScript, my previous background is Python/VBScript. Is it good enough? What best practices should I apply? Should I compress and nest the functions like this? Is it better to let each function do its thing and write different functions?

function doubleDigits(element){
return element.toLocaleString('en-EN', {
    minimumIntegerDigits: 2,
    useGrouping: false})
};

function displayDateTime(elements, id, separator){
    document.getElementById(id).innerHTML = elements.map(x => doubleDigits(x)).join(separator)
};

setInterval(setMultipleIntervals,200)    
function setMultipleIntervals(){
    // This function will update every other function that needs to be updated regularly.
    var time = new Date();
    let myFunctions = [displayDateTime([time.getHours(), time.getMinutes(), time.getSeconds()],'my_clock',':'),
                       displayDateTime([time.getUTCFullYear(), time.getMonth()+1, time.getDate()],'my_date','-')] 
    myFunctions.forEach(func => {try {func} catch{console.error(func);}})
;}
\$\endgroup\$
1
  • \$\begingroup\$ Could you clarify the direction to your question? Are you looking for JS best practices in general, or for general comments on your example code, or for comments on the function nesting in your example code specifically? \$\endgroup\$
    – Grilse
    Dec 24, 2021 at 3:34

4 Answers 4

0
\$\begingroup\$

"What best practices should I apply?" These suggestions are for JavaScript in a web browser (not node.js)

The number one best practice, in my opinion, is to test in every web browser you want to support. JavaScript has a lot of commonality across web browsers, but no two browsers are the same.

Number two: Whatever you can do in CSS, you should do in CSS. In particular, I would recommend that you try practicing CSS animations as early as you can; there's a lot of animation stuff I've written in JS and JQuery that I think I could have done with CSS (it's just not worth it to change at this point)

Number three: learn HTML 5: Specifically, I recommend learning about content-security-policy and all the different types (and built in input validation) in the INPUT tag.

\$\endgroup\$
1
  • \$\begingroup\$ Can you make any observations about the code itself? That is what code review is about. \$\endgroup\$
    – pacmaninbw
    Dec 24, 2021 at 13:54
0
\$\begingroup\$

General points.

  • Avoid redundant comments.
  • Never add redundant code. The last line of the interval callback does nothing. Why is it there???
  • Don't add text to elements via innerHTML use textContent.
  • Reserve the names element, elements for HTMLElements don't use them for string, numbers, arrays, etc...
  • Use 'const' for all variables that do not change.
  • Avoid setInterval as it is dangerous power hungry memory leak waiting to happen.

Don't abuse the client.

When you have code running on a client you are using their resources which they pay for.

  • Always write code to be as performant and efficient as possible.

    Use memory to store repeated operations that produce the same result. Eg query document once not each time the same element is needed.

  • Never run code if there is no need.

    You show seconds yet you update 200ms, 5 times above the required rate.

    Set Interval is automatically throttled when focus is lost (Because of its wide spread abuse) however what rate it runs at is determined by the browser. Best to actively control the rate of updates depending on focus/visibility.

Rewrites

First example uses focus and blur event to set update rates. Second example use visibilitychange change events to turn on and off updates.

The ideal is to use both focus/blur and visibilitychange to speed up, slowdown and stop updates.

Example 1

Slows update rate when focus is lost (note that lost focus does not mean that the page is not visible).

;(()=>{
    "use strict";
    const LOCALE_NUM = ['en-EN', {minimumIntegerDigits: 2, useGrouping: false}];
    const MIN_RATE = 5000;
    const MAX_RATE = 1000;
    const timeEl = document.querySelector("#clockEl");
    const dateEl = document.querySelector("#dateEl");
    var currentRate = MAX_RATE, updateHdl;

    addEventListener("focus", () => { currentRate = MAX_RATE; updateTime();} );
    addEventListener("blur", () => currentRate = MIN_RATE);
    const doubleDigits = num => num.toLocaleString(...LOCALE_NUM);
    const showArrVals = (el, vals, sep) => { el.textContent = vals.map(n => doubleDigits(n)).join(sep); }    
    updateTime();
    function updateTime() {
        clearTimeout(updateHdl);
        const date = new Date();
        showArrVals(timeEl, [date.getHours(), date.getMinutes(), date.getSeconds()], ':');
        showArrVals(dateEl, [date.getUTCFullYear(), date.getMonth() + 1, date.getDate()], '-');
        updateHdl = setTimeout(updateTime, currentRate);
    }
})();

Example 2

Constant rate time display will stop when page is hidden.

;(()=>{
    "use strict";
    const LOCALE_NUM = ['en-EN', {minimumIntegerDigits: 2, useGrouping: false}];
    const RATE = 1000;
    const timeEl = document.querySelector("#my_clock");
    const dateEl = document.querySelector("#my_date");
    var updateHdl;
    document.addEventListener("visibilitychange", () => {
      document.visibilityState === "hidden" ? clearTimeout(updateHdl) : updateTime();
    });
    const doubleDigits = num => num.toLocaleString(...LOCALE_NUM);
    const showArrVals = (el, vals, sep) => { el.textContent = vals.map(n => doubleDigits(n)).join(sep); }  
    updateTime();     
    function updateTime() {
        const date = new Date();
        showArrVals(timeEl, [date.getHours(), date.getMinutes(), date.getSeconds()], ':');
        showArrVals(dateEl, [date.getUTCFullYear(), date.getMonth() + 1, date.getDate()], '-');
        updateHdl = setTimeout(updateTime, RATE);
    }
})();
\$\endgroup\$
0
\$\begingroup\$

As you wrote it, the assignment let myFunctions = ... will create an array containing 2 times undefined, which is the result of the two displayDateTime() calls. Note that displayDateTime() has no explicit return statement, so it returns undefined.

Based on the myFunctions.forEach(...) that follows it, you seem to believe (incorrectly) that myFunctions contains functions that have not yet been executed, and then you attempt to execute them. But there are no functions to execute, that already happened when the array was created. As a result the forEach is basically a "no operation".

Here is the cleaned up code in a working snippet. I removed the unneeded array and forEach parts, and I renamed function 'setMultipleIntervals' to 'showDateAndTime' because I think that is a much better description of what it does.

function doubleDigits(element) {
  return element.toLocaleString('en-EN', {
    minimumIntegerDigits: 2,
    useGrouping: false
  });
};

function displayDateTime(elements, id, separator) {
  document.getElementById(id).innerHTML = elements
    .map(x => doubleDigits(x))
    .join(separator);
};

setInterval(showDateAndTime, 200);

function showDateAndTime() {
  var time = new Date();
  displayDateTime([time.getHours(), time.getMinutes(), time.getSeconds()], 'my_clock', ':'),
  displayDateTime([time.getUTCFullYear(), time.getMonth() + 1, time.getDate()], 'my_date', '-');
}
<div>Time: <b><span id="my_clock"></span></b></div>
<div>Date: <b><span id="my_date"></span></b></div>

\$\endgroup\$
0
\$\begingroup\$
elements.map(x => doubleDigits(x))

Array.map takes a function as an argument. You create a function which just calls another function. You can pass that other function directly instead.

elements.map(doubleDigits)
\$\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.