5
\$\begingroup\$

I have a solution but it feels bloated and unclean. How can I clean it up? The second for loop especially does not feel right.

I'm trying to calculate the opening hours but where we only use the highest opening time and the lowest closing time in a time-block. I will show you an example to clarify it.

If we have two time-blocks with

10:00-12:00 and 10:30-12:30

The result for the opening hour is going to be

10:30-12:00

Now imagine we have an array for a specific day:

[
    {
        "opens": "10:00",
        "closes": "12:00",
        "weekday": 5
    },
    {
        "opens": "16:00",
        "closes": "19:30",
        "weekday": 5
    },
    {
        "opens": "11:00",
        "closes": "12:30",
        "weekday": 5
    },
    {
        "opens": "17:00",
        "closes": "18:30",
        "weekday": 5
    }
]

Our opening hour should be for this day:

[
    {
        "opens": "11:00",
        "closes": "12:00"
    },
    {
        "opens": "17:00",
        "closes": "18:30"
    }
]

Now to my approach:

First I'm going to through the whole opening hour array for a specific day:

this.filteredTimes.forEach((time, index, array) => {...});

Inside the for loop, I'm saving the opens and closes time in MomentJS:

let opens = this.momentTime(time.opens); //saving it to moment
let closes = this.momentTime(time.closes); //moment(time, 'HH:mm')

After that, I'm again going through the whole array in another for loop:

for(let i = 0; i < array.length; i++){...} //array === this.filteredTimes

Inside the loop, I'm saving the opens and closes times for each time block again, but now I'm comparing it to the opens and closes time from the first for loop.

let arrayOpens = this.momentTime(array[i].opens);
let arrayCloses = this.momentTime(array[i].closes);

if(arrayOpens.isBetween(opens, closes)){
   opens = arrayOpens.isAfter(opens) ? arrayOpens : opens;
}

If the arrayOpens is between the saved time block, we can save a new opens time if our condition is true.

After that, we are going to save the new opens and closes time and we try to avoid duplication:

let hour = {
    opens: opens.format('HH:mm'),
    closes: closes.format('HH:mm')
};

if(! openingHour.find(element => element.opens == hour.opens && element.closes == hour.closes)){
    openingHour.push(hour);
}

The whole code is here to test (or on JsFiddle):

moment().format();


let filteredTimes = [
	{
    "opens": "10:00",
    "closes": "12:00",
    "weekday": 5
  },
  {
    "opens": "16:00",
    "closes": "19:30",
    "weekday": 5
  },
  {
    "opens": "11:00",
    "closes": "12:30",
    "weekday": 5
  },
  {
    "opens": "17:00",
    "closes": "18:30",
    "weekday": 5
  }
];

let openingHour = [];

calculate();


function calculate(){

  filteredTimes.forEach((time, index, array) => {
    let opens = momentTime(time.opens);
    let closes = momentTime(time.closes);

    for(let i = 0; i < array.length; i++){
      let arrayOpens = momentTime(array[i].opens);
      let arrayCloses = momentTime(array[i].closes);

      if(arrayOpens.isBetween(opens, closes)){
        opens  = arrayOpens.isAfter(opens) ? arrayOpens : opens;
      }
      if(arrayCloses.isBetween(opens, closes)){
        closes  = arrayCloses.isBefore(closes) ? arrayCloses : closes;
      }
    }

    let hour = {
      opens: opens.format('HH:mm'),
      closes: closes.format('HH:mm')
    };

    if(! openingHour.find(element => element.opens == hour.opens && element.closes == hour.closes)){

      openingHour.push(hour);
    }

  });
  
  console.log(openingHour);
  return openingHour;
}

function momentTime(time){
	return moment(time, 'HH:mm');
}
<script src="https://cdnjs.cloudflare.com/ajax/libs/moment.js/2.22.2/moment.min.js"></script>

\$\endgroup\$
  • \$\begingroup\$ Have you considered sorting the opening/closing times, possibly using something like h*60+m as the sort value? \$\endgroup\$ – Barry Carter Jun 20 '18 at 17:09
  • \$\begingroup\$ I'm misunderstanding something here. If two time blocks don't overlap, shouldn't the opening/closing become null, since the earliest closes time is later than the latest opens time? Or is that a special case? \$\endgroup\$ – Barry Carter Jun 20 '18 at 17:19
  • \$\begingroup\$ @BarryCarter sorry I could not answer straight away. I try to explain how these opening hours even exists. Imagine you are able to sell something. Every time you sell something, I need to know what your preferred pickup times are (it's the same as opening hours). If you sell another thing, you need to write down another pickup times. So that is why I need to try to find the difference between those times. (And it's just a reference point, the buyer can chat with him and of course choose to come to another time). \$\endgroup\$ – Shadrix Jun 21 '18 at 9:17
  • \$\begingroup\$ @BarryCarter and because the user has different "opening hours" in a day. (In the morning the user has got time until 9, after that he is not at home, but at 18:00 he is ready again). And the opening/closing times are sorted with array.sort() \$\endgroup\$ – Shadrix Jun 21 '18 at 9:19
  • \$\begingroup\$ You might want to condense your question into 1) the context/use case/ scenario 2) your code. You do not need to explain how you're doing it, the code pretty much does that for you. \$\endgroup\$ – Joseph Jun 21 '18 at 20:58
2
\$\begingroup\$
  • calculate is not reusable. It's hardcoded to always use filteredTimes. It's also putting its results to an array on the outside. Strive for pure functions as much as possible, like math equations. It keeps them reusable, testable and deterministic.

  • Your variable names are vague. A good sign that you're not naming things well enough is when you have to explain to another developer what it is (like what you're doing in the comments). Never be afraid to name things verbosely.

  • You might not need moment.js. What you're doing with moment.js is just intermediary conversion of formats - an unnecessary procedure. When operating with dates and times, it's best to have your data in a format that doesn't need intermediate conversions.

Now simply put, what you're essentially looking for "windows" where the given times overlap. Here's a modified version of your code. The modifications include:

  • The times are all numbers. You don't really need to have the HH:mm format in this operation. If you strip off the : from the time, they're effectively numbers from 0000 to 2359` (with the last 40 mins in each hour missing). This eliminates the need for moment.js.

  • Nested array.forEach(), because for loops are also clunky (you have to keep indexes, do length comparisons, etc.). Plus you're using array.forEach() already anyways.

  • I stored results in an object keyed by the combination of from and to. This way, the lookup doesn't require a very clunky array.find(). Converting it to an array is as simple as iterating over the keys and mapping them to the values.

  • computeWindows is now a pure-ish function. You can now call it anytime, anywhere with any input, and always get a deterministic-ish output (Object.keys() do not guarantee the order of keys, so while the items may be the same, the order may be different by browser).

const ranges = [{
  from: 1000,
  to: 1100
}, {
  from: 1015,
  to: 1115
}, {
  from: 1030,
  to: 1130
}, {
  from: 1045,
  to: 1145
}, {
  from: 1100,
  to: 1200
}]

const isBetween = (value, start, end) => value > start && value < end

const computeWindows = ranges => {
  const index = {}
  
  ranges.forEach(({ from: rFrom, to: rTo }) => {  
    ranges.forEach(({ from: cFrom, to: cTo }) => {
      // The actual logic
      rFrom = isBetween(cFrom, rFrom, rTo) && cFrom > rFrom ? cFrom : rFrom
      rTo = isBetween(cTo, rFrom, rTo) && cTo < rTo ? cTo : rTo
    })
    
    // Construct the key and value
    const key = `${rFrom}-${rTo}`    
    const value = { from: rFrom, to: rTo }
    
    // Use existing value of key or add new value to key.
    index[key] = index[key] || value
  })

  // Convert results into an array of ranges.
  return Object.keys(index).map(k => index[k])
}

console.log(computeWindows(ranges))

\$\endgroup\$
  • \$\begingroup\$ I learned a lot! One last thing I'm wondering. You said I could remove the ":" to calculate every time as a number. However, I'm wondering what if I don't remove it. Is it still reliable? Chrome still calculates "12:30">"12:00" as true and "12:00">"12:30" as false \$\endgroup\$ – Shadrix Jun 23 '18 at 14:44
  • 1
    \$\begingroup\$ @Shadrix String comparison is alphabetical comparison. "1:30" > "12:30" results totrue which is incorrect from a time point of view. But : is greater than 2 alphabetically. You won't notice this bug immediately, even worse if you hide the values behind variables. Stick to numbers. Even moment.js is probably comparing time in numbers, millisecond timestamps possibly. \$\endgroup\$ – Joseph Jun 23 '18 at 21:16
  • \$\begingroup\$ What did you use to develop your personal website? ( will delete this comment when I see you read it, thanks) \$\endgroup\$ – Billal Begueradj Jun 29 '18 at 10:18

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.