2
\$\begingroup\$

When creating a function to be reusable throughout the project I came across something strange. After looking at this function it looks very much like it could be refactored.

The function should only return the short month and day, in that order. If it receives a parameter, it will be a Firestore Timestamp with the following format:

{
   "seconds":1667420699,
   "nanoseconds":394000000
}

If it does not receive a parameter, it must return today's month and day. So I did this:

function shortMonthDay(timestamp = undefined) {
  let date

  if (timestamp) {
    date = new Date(timestamp.seconds * 1000 + timestamp.nanoseconds / 1000000)
  } else {
    date = new Date()
  }

  return date.toLocaleDateString('en-US', {
    month: 'short',
    day: 'numeric'
  }) // Nov 2
}

The function works as it should, it doesn't return any errors. But I wonder if it could be improved and how.

\$\endgroup\$
5
  • 1
    \$\begingroup\$ Is the number of nanoseconds in the current second somehow relevant to what day it is? \$\endgroup\$
    – slepic
    Dec 2, 2022 at 19:39
  • \$\begingroup\$ I hadn't tested it without the nanoseconds before, but it actually doesn't seem to make that much of a difference. But it is a fact that JSON will come with nanoseconds. \$\endgroup\$
    – ARNON
    Dec 2, 2022 at 20:20
  • \$\begingroup\$ @radarbob why would it matter? He's not returning the Date object... He just wants the string and uses Date internally to build it. There's no comparison of dates. \$\endgroup\$
    – slepic
    Dec 3, 2022 at 8:52
  • \$\begingroup\$ ARNON I wanted to say you can omit nanoseconds from your calculation, even if it's coming from Api, it is irrelevant for this method. \$\endgroup\$
    – slepic
    Dec 3, 2022 at 8:54
  • \$\begingroup\$ Makes sense, thanks for it. About refactoring the function: would you do anything? \$\endgroup\$
    – ARNON
    Dec 3, 2022 at 12:57

2 Answers 2

2
\$\begingroup\$

Minor points

  • Default parameters eg shortMonthDay(timestamp = undefined) are only assigned to arguments that are undefined or contain undefined. To assign the default undefined will do nothing and is just code noise.

  • The timestamp object is not vetted. If it contains seconds, it may not have nanoseconds (and visa versa), or either or both may not hold a number. You need to ensure that you do not pass NaN to the Date constructor when parsing the timestamp

Function roles

shortMonthDay would be better as two functions.

  1. timestampToDate to convert a timestamp if given to a date.
  2. formatDate to format the date object, which you can curry with the locale format. Thus you don't need to touch the timestampToDate to change the output format

Rewrite

We can rewrite the function as follows

const formatDate = (loc = "en-US", format = {month: "short", day: "numeric"}) => 
  date => date.toLocaleDateString(loc, format);
const timestampToDate = (timestamp = {}) => new Date(
  (isNaN(timestamp.seconds) ? Date.now() : Number(timestamp.seconds) * 1000) +
  (isNaN(timestamp.nanoseconds) ? 0 : timestamp.nanoseconds * 1e-6)
);    
const toMonthDay = formatDate();

// basic tests
const timestamps = [undefined,   // current local date
  {seconds: 1670263910, nanoseconds: 394000000}, // ~ Dec 6 GMT+0800 for rest
  {seconds: 1670263910, nanoseconds: "blah"},
  {seconds: 1670263910},
  {seconds: "foo", nanoseconds: 1667420699000 + 394}
].forEach(timestamp => console.log(toMonthDay(timestampToDate(timestamp))));

Note As a habit I avoid divides as generally divides are slower than multiplies. Its just a habit, in this case the difference between * 1e-6 and / 1e6 is inconsequential.

Note In timestampToDate the names are a little noisy and because the variable timestamp is being used so often one could also write it as

const timestampToDate = (stamp = {}) => new Date(
    (isNaN(stamp.seconds) ? Date.now() : Number(stamp.seconds) * 1000) +
    (isNaN(stamp.nanoseconds) ? 0 : stamp.nanoseconds * 1e-6)
  );    
\$\endgroup\$
1
  • \$\begingroup\$ I really appreciate your answer. It is really complete e I learned with it. Thanks. \$\endgroup\$
    – ARNON
    Dec 5, 2022 at 21:25
1
\$\begingroup\$

Considering the discussion from the comment if nanoseconds matter default value can be passed with || if timestamp don't have any of the property.

Can be demonstrated as

function shortMonthDay(timestamp) {
    let date = new Date(timestamp?.seconds * 1000 || Date.now() + (timestamp?.nanoseconds || 0) / 1000000);
    return date.toLocaleDateString('en-US', {
        month: 'short',
        day: 'numeric'
    })
}

const timestamp = {
    "seconds": 1667420699,
    "nanoseconds": 394000000
};

console.log(`with timestamp ${shortMonthDay(timestamp)}`);
console.log(`without timestamp ${shortMonthDay()}`);
.as-console-wrapper { max-height: 100% !important; top: 0; }

Without the timestamp parameter it compromises nanoseconds to 0 and seconds to the current timestamp.


Performance

with old snippet

function shortMonthDay(timestamp = undefined) {
  let date

  if (timestamp) {
    date = new Date(timestamp.seconds * 1000 + timestamp.nanoseconds / 1000000)
  } else {
    date = new Date()
  }

  return date.toLocaleDateString('en-US', {
    month: 'short',
    day: 'numeric'
  }) // Nov 2
}

const timestamp = {
    "seconds": 1667420699,
    "nanoseconds": 394000000
};

let res = [];
for(let i = 0; i< 10; i++){
  let start = window.performance.now();
  shortMonthDay(timestamp);
  res.push(window.performance.now() - start);
}
console.log(res);
.as-console-wrapper { max-height: 100% !important; top: 0; }

With the altered snippet

function shortMonthDay(timestamp) {
    let date = new Date(timestamp?.seconds * 1000 || Date.now() + (timestamp?.nanoseconds || 0) / 1000000);
    return date.toLocaleDateString('en-US', {
        month: 'short',
        day: 'numeric'
    })
}

const timestamp = {
    "seconds": 1667420699,
    "nanoseconds": 394000000
};

let res = [];
for(let i = 0; i< 10; i++){
  let start = window.performance.now();
shortMonthDay(timestamp);
res.push(window.performance.now() - start);
}
console.log(res);
.as-console-wrapper { max-height: 100% !important; top: 0; }

\$\endgroup\$
2
  • \$\begingroup\$ Your solution is valid, but when it doesn't receive any parameters, I look at this syntax new Date(Date.now() + 0) and it looks different to me. I didn't even know this would work until I tested it 1 minute ago. lol \$\endgroup\$
    – ARNON
    Dec 4, 2022 at 20:26
  • 1
    \$\begingroup\$ @ARNON Even many professionals wouldn't be sure whether that's valid. In first place, make your code understandable at a glance. Only when you get significant performance gain, you should sacrifice readability. But the performance gain here is miniscule if not nonexistent and the provided benchmark is not reliable. \$\endgroup\$
    – slepic
    Dec 6, 2022 at 17:28

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service and acknowledge you have read our privacy policy.

Not the answer you're looking for? Browse other questions tagged or ask your own question.