trjhtr

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>

• 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 ) => )

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


console.log(computeWindows(ranges))