0
\$\begingroup\$

This is a function which aims to convert an amount of milliseconds to a more human-interpretable Day(s) Hour(s) Minute(s) Second(s) format:

function dhms(t) {
    d = Math.floor(t / (1000 * 60 * 60 * 24)),
    h = Math.floor((t % (1000 * 60 * 60 * 24)) / (1000 * 60 * 60)),
    m = Math.floor((t % (1000 * 60 * 60)) / (1000 * 60)),
    s = Math.floor((t % (1000 * 60)) / 1000);
    return d + 'Day(s) ' + h + 'Hour(s) ' + m + 'Minute(s) ' + s + 'Second(s)'
}

So for getting the variables values I have for now something quite verbose:

d = Math.floor(t / (1000 * 60 * 60 * 24)),
h = Math.floor((t % (1000 * 60 * 60 * 24)) / (1000 * 60 * 60)),
m = Math.floor((t % (1000 * 60 * 60)) / (1000 * 60)),
s = Math.floor((t % (1000 * 60)) / 1000);

Is it the way to go? Or should I go with:

d = Math.floor(t / 86400000), 
h = Math.floor(t % 86400000 / 3600000), 
m = Math.floor(t % 3600000 / 60000), 
s = Math.floor(t % 60000 / 1000);

Or even with:

d = Math.floor(t / 864e5), 
h = Math.floor(t % 864e5 / 36e5), 
m = Math.floor(t % 36e5 / 6e4), 
s = Math.floor(t % 6e4 / 1e3);

Or another way? How is it recommended to assign time values?

\$\endgroup\$

closed as off-topic by πάντα ῥεῖ, Toby Speight, Graipher, pacmaninbw, esote Apr 7 at 20:52

This question appears to be off-topic. The users who voted to close gave this specific reason:

  • "Lacks concrete context: Code Review requires concrete code from a project, with sufficient context for reviewers to understand how that code is used. Pseudocode, stub code, hypothetical code, obfuscated code, and generic best practices are outside the scope of this site." – πάντα ῥεῖ, Toby Speight, Graipher, pacmaninbw, esote
If this question can be reworded to fit the rules in the help center, please edit the question.

  • 3
    \$\begingroup\$ To me this feels like stub code, and lacks sufficient context. I have no clue what this code is supposed to achieve and I wouldn't be surprised that there are better ways to achieve its goals by using built-in functionality, but the lack of context prevents us knowing. (What is t?) See also codereview.meta.stackexchange.com/questions/3649/… \$\endgroup\$ – BCdotWEB Apr 5 at 10:55
  • 1
    \$\begingroup\$ @BCdotWEB is it really that hard to figure out what t is? Strong clue: he's dividing it by 1000*60*60*24 and assigning the result to a variable named d, in a language that dispenses timestamps in milliseconds. \$\endgroup\$ – Oh My Goodness Apr 5 at 11:51
  • 2
    \$\begingroup\$ Even though the context might be easy to guess in this specific example, the request for context exists for a good reason. Since your just presenting your solution but not the actual problem, it's hard to say if there might be a more appropriate solution. See also this SO meta post. \$\endgroup\$ – AlexV Apr 5 at 12:47
  • 1
    \$\begingroup\$ FYI github.com/moment/moment/blob/develop/moment.js has the following comment in their library: //using 1000 * 60 * 60 instead of 36e5 to avoid floating point rounding errors https://github.com/moment/moment/issues/2978, so are you sure all three implementations are correct and result in the same output? \$\endgroup\$ – BCdotWEB Apr 5 at 13:48
  • \$\begingroup\$ @BCdotWEB The fp error is not in the format 35e5 it is in the number 360000 eg 2.3 * (1000*60*60) === 2.3 * 36e5, 2.3*1000*60*60 !== 2.3 * 36e5, 2.3*3600000 !== 2.3*1000*60*60, 2.3*1000*60*60 !== 60*60*1000*2.3 and 1000*60*60 === 36e5 are all true. The Op does not multiply, making the point mute. \$\endgroup\$ – Blindman67 Apr 5 at 16:42
1
\$\begingroup\$

The Decimal Point

Exponential notation is by far the better for large numbers.

However when writing numbers in this format the convention is to have the decimal point after the first digit, as the exponent represents the magnitude of the value which is obscured if you have to locate the decimal point.

d = Math.floor(t / 8.64e7), 
h = Math.floor(t % 8.64e7 / 3.6e6), 
m = Math.floor(t % 3.6e6 / 6e4), 
s = Math.floor(t % 6e4 / 1e3);

As constants

But.. These are magic numbers, and you repeat some of them several times, which is prone to error. Also assuming that this function would be part of a set of such functions declaring these constants as named variables would be much better.

DAY_MS = 8.64e7;
HOUR_MS = 3.6e6;
MIN_MS = 6e4;
SEC_MS = 1e3;

Derived values

As they are derived from each other you can then write it as

SEC_MS = 1e3;
MIN_MS = SEC_MS * 60;
HOUR_MS = MIN_MS * 60;
DAY_MS = HOUR_MS * 24;

Encapsulate

If you then encapsulate them you can drop the _MS and your function would look like.

"use strict";
const dhms = (()=>{
    const SEC = 1e3;
    const MIN = SEC * 60;
    const HOUR = MIN * 60;
    const DAY = HOUR * 24;
    return time => {
        const ms = Math.abs(time);
        const d = ms / DAY | 0;
        const h = ms % DAY / HOUR | 0;
        const m = ms % HOUR / MIN | 0;
        const s = ms % MIN / SEC | 0;
        return `${time < 0 ? "-" : ""}${d}Day(s) ${h}Hour(s) ${m}Minute(s) ${s}Second(s)`;
    };
})();

Notes

  1. That t is made positive ms to avoid a negative sign on each number. This also lets you use the shorter and quicker | 0 (bit-wise OR 0) to floor the values, which you should only use for positive integers.

  2. The use of a template string to format the output.

  3. You could also define it as a module and thus avoid the need to encapsulate the constants as a module has its own local scope.

\$\endgroup\$