4
\$\begingroup\$

I need to fill missing business days (i.e. any days except saturday and sunday) in a series. So let's say I have this array:

const arr =  [
    "Date,Value",
    "2020-12-31,1",
    "2021-01-01,-3",
    "2021-01-04,-38",
    "2021-01-05,-14",
    "2021-01-07,27",
    "2021-01-08,55",
]

"2021-01-06" is a wednesday, but is missing in the array. What I need to do then is to take the value from the day before (-14) and use it for "2021-01-06". At the end it would look like this:

[
    "Date,Value",
    "2020-12-31,1",
    "2021-01-01,-3",
    "2021-01-04,-38",
    "2021-01-05,-14",
    "2021-01-06,-14",
    "2021-01-07,27",
    "2021-01-08,55",
]

Currently Im using this algorithm (which seems to work), but I wonder if it can be done more efficiently:

const fixEmptyBusinessDays = (arr) => {
    const newArr = [];
    if(arr.length){
        newArr.push(arr[0]);
        arr.shift();
        let latestValue = 0;
        for(let i = 0;i<arr.length;i++){
            const el = arr[i];
            if(el && el.length){
                newArr.push(el);
                if(el.length > 1){
                    const val = el[1];
                    latestValue = val;
                }
                const currDate = el[0];
                const currDateAsDateObject = new Date(currDate);
                if(currDate && currDateAsDateObject){
                    const day = currDateAsDateObject.getDay();
                    if((i+1)<arr.length && arr[i+1] && arr[i+1].length){
                        const next = arr[i+1];
                        const nextDayInFile = next[0];
                        const nextWeekDayUnformatted = get_next_weekday(new Date(currDate));
                        const nextMonth = (nextWeekDayUnformatted.getMonth()+1) < 10 ? `0${(nextWeekDayUnformatted.getMonth()+1)}` : (nextWeekDayUnformatted.getMonth()+1);
                        const nextDay = nextWeekDayUnformatted.getDate() < 10 ? `0${nextWeekDayUnformatted.getDate()}` : nextWeekDayUnformatted.getDate();
                        const nextWeekDay = `${nextWeekDayUnformatted.getFullYear()}-${nextMonth}-${nextDay}`
                        if(nextDayInFile !== nextWeekDay){
                            newArr.push([nextWeekDay, latestValue]);
                        }
                    }
                }
            }
        }
    }
    return newArr;
}
const arr =  [
    "Date,Value",
    "2020-12-31,1",
    "2021-01-01,-3",
    "2021-01-04,-38",
    "2021-01-05,-14",
    "2021-01-07,27",
    "2021-01-08,55",
]
const result = fixEmptyBusinessDays(arr);
\$\endgroup\$
2
  • 1
    \$\begingroup\$ Very nice question, is this a school assignment? \$\endgroup\$
    – konijn
    Dec 20, 2021 at 16:30
  • 1
    \$\begingroup\$ @konijn: It is obviously not a school assignment. Why did you think it was? The OP is solving a real problem. \$\endgroup\$
    – peterSO
    Dec 21, 2021 at 12:11

3 Answers 3

4
\$\begingroup\$

How do you guarantee original string-dates array is in ascending (descending?) order? I'd bet out of order dates will break the code.

I wonder if it can be done more efficiently

The code and its structure is so abstruse that I cannot tell if the above concern is addressed. It is a Mariana trench of nesting; an "everything all at once" kind of construction. So if maintainable code is efficient (and it is!) then yeah, this could be more efficient:

I think each bullet point below can be a function call from a "main" function. Anything done more than once, like sorting, should be its own function. It may not seem "efficient" but refactoring nested iterating into separate functions will be readable, understandable, testable, and changeable.

  • Up front, turn those date strings into an array of JS Date objects and sort them.

  • Iterate the workDates in pairs

    • From the pair start to end dates (now guaranteed to be in order) creating new dates for all missing workdays, dropping weekends of course. As a separate function this could be used to generate months or years of new work dates.
    • Use a new array to collect all of the newly created dates. Don't add them to the "source" array yet. Changing an array during iteration can cause problems.
  • Push new dates onto the end of the original array

  • Resort the updated 'workDates'

  • Iterate the 'workDates' outputting the desired string format.

\$\endgroup\$
2
\$\begingroup\$

Code should be correct, maintainable, robust, reasonably efficient, and, most importantly, readable.


It helps to provide some real-life cases as examples. For example, I do this for comparing investment returns. The NYSE and other exchanges are open for trading Monday to Friday, except for holidays and a few other special days. For daily closing price comparablity, I extrapolate the missing Monday to Friday (business) days by using the closing price from the previous trading day.


MDN Web Docs: Date() constructor

Parameters

dateString

A string value representing a date, specified in a format recognized by the Date.parse() method. (These formats are IETF-compliant RFC 2822 timestamps, and also strings in a version of ISO8601.)

Support for ISO 8601 formats differs in that date-only strings (e.g. "1970-01-01") are treated as UTC, not local.

You write,

const currDateAsDateObject = new Date(currDate);

where currDate is an ISO8601 string, for example "2020-12-31". Therefore, currDateAsDateObject is treated as a UTC date.

To calculate the next date, you use Date getFullYear, getMonth, getDate, and getDay which return relative local time. For UTC date arithmetic, use Date getUTCFullYear, getUTCMonth, getUTCDate, and getUTCDay which return absolute universal time.


Readable code has organization and structure. Your sprawling code appears to favor stream-of-consciousness.

The world is imperfect. Handle imperfect data. "be conservative in what you do, be liberal in what you accept from others", Postel's law.


Here's a quickly scribbled first draft to illustrate organization, structure, and handling imperfect data. There is room for improvement.


missingdays.js:

const allBusinessDays = (input) => {
  const error = undefined;

  const parseISODate = (string) => {
    const date = new Date(string);
    if (isNaN(date)) {
      return error;
    }
    if (date.toISOString().substr(0, 10) !== string) {
      return error;
    }
    return date;
  }

  const parseLine = (line) => {
    let [date, value] = [error, error];
    const fields = line.split(",");
    if (fields.length > 0) {
      date = parseISODate(fields[0]);
    }
    if (fields.length > 1) {
      value = fields[1];
    }
    return [date, value];
  }

  const extrapolateDays = (prev, curr) => {
    const [pDate, pValue] = parseLine(prev);
    const [cDate, cValue] = parseLine(curr);
    if (pDate === error || cDate === error) {
      output.push(curr);
      return curr;
    }

    while (true) {
      pDate.setUTCDate(pDate.getUTCDate() + 1);
      if (pDate >= cDate) {
        break;
      }

      const pUTCDay = pDate.getUTCDay();
      if (1 > pUTCDay || pUTCDay > 5) {
        continue;
      }

      let missing = pDate.toISOString().substr(0, 10) + "," + pValue;
      output.push(missing);
    }

    output.push(curr);
    return curr;
  }

  let output = [];

  if (input.length <= 0) {
    return output;
  }
  output.push(input.shift());

  if (input.length <= 0) {
    return output;
  }
  input.sort();
  const first = input[0];
  input.reduce(extrapolateDays, first);

  return output;
}

const trading = [
  "Date,Value",
  "2020-12-31,1",
  "2021-01-01,-3",
  "2021-01-04,-38",
  "2021-01-05,-14",
  "2021-01-07,27",
  "2021-01-08,55",
];

console.log("Trading Days:");
console.log(trading);
const business = allBusinessDays(trading);
console.log("Business Days:");
console.log(business);

$ node missingdays.js

Trading Days:
[
  'Date,Value',
  '2020-12-31,1',
  '2021-01-01,-3',
  '2021-01-04,-38',
  '2021-01-05,-14',
  '2021-01-07,27',
  '2021-01-08,55'
]
Business Days:
[
  'Date,Value',
  '2020-12-31,1',
  '2021-01-01,-3',
  '2021-01-04,-38',
  '2021-01-05,-14',
  '2021-01-06,-14',
  '2021-01-07,27',
  '2021-01-08,55'
]

\$\endgroup\$
6
  • \$\begingroup\$ I am not sure "handle imperfect data" is always good advice. In fact, in a well-architected environment, this is bad advice. \$\endgroup\$
    – konijn
    Dec 21, 2021 at 8:57
  • \$\begingroup\$ @konijn: I am sure. The world is not a well-architected environment. Deal with it. \$\endgroup\$
    – peterSO
    Dec 21, 2021 at 12:08
  • \$\begingroup\$ "imperfect data", as I read it, means the opposite of assuming data is perfect. Absent prudent up-front validation I can easily imagine (I don't have to imagine, actually), deep in the bowels of some if-for-if-if-etc-etc nest, suddenly discovering some data is "imperfect" and convulsively hacking into a recursive nightmare of fix-break-fix up and down that valley of the shadow of inflexible code death. \$\endgroup\$
    – radarbob
    Dec 22, 2021 at 2:53
  • \$\begingroup\$ @radarbob: You have a vivid imagination, an apocalyptic vision. Perfection is rare. We learn to deal with imperfection. The internet was designed to withstand a nuclear war. You asked in your answer to the OP, "[the] array is in ascending (descending?) order?" In fact, real-world data may be in ascending or descending order. I handle the imperfection with a sort. \$\endgroup\$
    – peterSO
    Dec 22, 2021 at 19:47
  • \$\begingroup\$ @radarbob: The apocalyptic style of your comment about imperfect data reminded me of this wonderful apocalyptic style answer to a Stack Overflow question: HTML is not a regular language. \$\endgroup\$
    – peterSO
    Dec 23, 2021 at 5:24
1
\$\begingroup\$

A medium-sized review;

  • Not a huge fan of arr and el and newArr, I suggest to either go for fully spelled out names for readability or spartan variable naming. I go for list and v(alue)

  • Its considered bad form to change the list you receive as input, especially if the function name does not tell you it does that

  • I think addEmptyBusinessDays is more evocative than fixEmptyBusinessDays

  • This part confuses me;

          if(el && el.length){
              newArr.push(el);
              if(el.length > 1){
    

    Why check for (el.length > 1) if you already checked truthiness with el.length

  • I suggest using libraries for date manipulation, my counterexample uses moment.js and moment-business-days.js. You should do research to find what fits you.

  • You can use a single comma-separated const statement instead of 6 different const statements

  • I am surprised that nobody complained about get_next_weekday being absent in your code ;) I would just push for lowerCamelCase, so get_next_weekday -> getNextWeekday

  • Radarbob is right, you are using the arrow anti-pattern;

                        }
                    }
                }
            }
        }
    } 
    

This is my personal attempt at this;

const list =  [
    "Date,Value",
    "2020-12-31,1",
    "2021-01-01,-3",
    "2021-01-04,-38",
    "2021-01-05,-14",
    "2021-01-07,27",
    "2021-01-08,55",
]

function fillDateValues(dateValues){

  const 
  dateFormat = "YYYY-MM-DD", //If tomorrow the date format changes..
  dateMap = dateValues.map(v=>v.split(",")), //nicer format, and this way I don't modify dateValues
  out = [dateMap.shift().join(",")], //it makes this though a tad uglier
  last = dateMap.pop(),
  lastDate = moment(last[0], dateFormat);
  
  let
  current = dateMap.shift(),
  currentDate = moment(current[0], dateFormat);
   
  while(lastDate.diff(currentDate)>0){
    const formattedDate = currentDate.format(dateFormat);
    current = dateMap.find(item=>(item[0]==formattedDate)) || current;
    out.push(`${formattedDate},${current[1]}`);
    currentDate = currentDate.businessAdd(1);
  }
  //I could have calculated the last position to determine `last`, then I would not need this `push`, this is why I like this question, there's so many ways to write this, and perfection really is subjective in this one
  out.push(last.join(","));

  return out;
}

console.log(fillDateValues(list));
<script src="https://cdnjs.cloudflare.com/ajax/libs/moment.js/2.29.1/moment.min.js"></script>
<script src="https://cdnjs.cloudflare.com/ajax/libs/moment-business-days/1.2.0/index.min.js"></script>

\$\endgroup\$
2
  • \$\begingroup\$ In your counterexample you use moment.js and moment-business-days.js. The moment.js experts say, "we would like to discourage Moment from being used in new projects going forward" and "we now generally consider Moment to be a legacy project." \$\endgroup\$
    – peterSO
    Dec 22, 2021 at 19:30
  • \$\begingroup\$ The OP asked if "it can be done more efficiently." Using a 2,557 element array of real-life data, I ran a Node.js benchmark using your code. The results were around 131ms. Since the OP's code is incomplete, I ran the same benchmark using my code. The results were around 18ms. \$\endgroup\$
    – peterSO
    Dec 22, 2021 at 19:36

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.