patternjavascriptMinor
Recurring dates within a range
Viewed 0 times
withinrangedatesrecurring
Problem
I'm using this code to find out the recurring dates, within a given range. Is this the correct way to find out the recurring dates within a range of dates?
between[] contains all the dates calculated within the range provided by the user. I only pasted the code that is relevant to the recurrence part only.switch (true) {
case (interval == 7):
//weekly
//Push in the selected dates in the selected array.
for (var i = 0; i < between.length; i += 7) {
selected.push(between[i]);
}
break;
case (interval == 30):
//Monthly
//Push in the selected dates in the selected array.
for (var i = 0; i < between.length; i += 30) {
selected.push(between[i]);
}
break;
case (interval == 15):
//Bi-Monthly
//Push in the selected dates in the selected array.
for (var i = 1; i < between.length; i += 15) {
selected.push(between[i]);
}
break;
case (interval == 0):
//One time event
//Push in the selected dates in the selected array.
for (var i = 0; i < 1; i++) {
selected.push(between[i]);
}
break;
default:
return undefined;
}Solution
TL;DR; skip to if/else if/else statements
I can't say much because there is so little context, it looks like your code functions properly and efficiently,
Your indentation on the other hand is a little weird to me, I would do it like this with out so many tabs and new lines
EDIT:
Mateon1's answer points out a
after removing indentation I noticed that your one time event is smelly, you should just select the first date
you use more overhead for a
After all that I also removed some unneeded comments they were just saying the same thing that the code was telling me outright.
Here is the Code that is left
if/else if/else statements
After viewing the other answer, I think that this would be much better written as an if/else if/else statement like the following
and since I have gone this far I think that we could actually create a nice little function that does all of this when given an interval, I included the tests to make sure that the interval stays within the constraints but you could alter that to make it much more extendable.
From @Rolfl's answer I noticed that I too missed the 0-base array bug, so here is the function with the fix
I wrote it like that so that the user doesn't have to know that the code is based on a 0-based array.
You might want to look into using JavaScript's Built in
I can't say much because there is so little context, it looks like your code functions properly and efficiently,
Your indentation on the other hand is a little weird to me, I would do it like this with out so many tabs and new lines
switch (true) {
case (interval == 7):
//weekly
//Push in the selected dates in the selected array.
for (var i = 0; i < between.length; i += 7) {
selected.push(between[i]);
}
break;
case (interval == 30):
//Monthly
//Push in the selected dates in the selected array.
for (var i = 0; i < between.length; i += 30) {
selected.push(between[i]);
}
break;
case (interval == 15):
//Bi-Monthly
//Push in the selected dates in the selected array.
for (var i = 1; i < between.length; i += 15) {
selected.push(between[i]);
}
break;
case (interval == 0):
//One time event
//Push in the selected dates in the selected array.
for (var i = 0; i < 1; i++) {
selected.push(between[i]);
}
break;
default:
return undefined;
}EDIT:
Mateon1's answer points out a
break; that isn't indented correctly, I have fixed this in my code snippet.after removing indentation I noticed that your one time event is smelly, you should just select the first date
case (interval == 0):
//One time event
//Push in the selected dates in the selected array.
selected.push(between[0]);
break;you use more overhead for a
for loop than is necessary, don't get into the habit of using bigger things than you need.After all that I also removed some unneeded comments they were just saying the same thing that the code was telling me outright.
Here is the Code that is left
switch (true) {
case (interval == 7):
//weekly
for (var i = 0; i < between.length; i += 7) {
selected.push(between[i]);
}
break;
case (interval == 30):
//Monthly
for (var i = 0; i < between.length; i += 30) {
selected.push(between[i]);
}
break;
case (interval == 15):
//Bi-Monthly
for (var i = 1; i < between.length; i += 15) {
selected.push(between[i]);
}
break;
case (interval == 0):
//One time event
selected.push(between[0]);
break;
default:
return undefined;
}if/else if/else statements
After viewing the other answer, I think that this would be much better written as an if/else if/else statement like the following
if (interval == 7) {
for (var i = 0; i < between.length; i += 7) {
selected.push(between[i]);
}
} else if (interval == 15) {
for (var i = 0; i < between.length; i += 15) {
selected.push(between[i]);
}
} else if (interval == 30) {
for (var i = 0; i < between.length; i += 30) {
selected.push(between[i]);
}
} else if (interval == 0) {
selected.push(between[0]);
} else {
return undefined;
}and since I have gone this far I think that we could actually create a nice little function that does all of this when given an interval, I included the tests to make sure that the interval stays within the constraints but you could alter that to make it much more extendable.
function GetRecurringDates(interval) {
if (interval != 0 || interval != 7 || interval != 15 || interval != 30) {
return undefined;
}
for (var i = 0; i < between.length; i += interval) {
selected.push(between[i]);
}
}From @Rolfl's answer I noticed that I too missed the 0-base array bug, so here is the function with the fix
function GetRecurringDates(interval) {
if (interval != 0 || interval != 7 || interval != 15 || interval != 30) {
return undefined;
}
for (var i = 0; i < between.length - 1; i += interval) {
selected.push(between[i]);
}
}I wrote it like that so that the user doesn't have to know that the code is based on a 0-based array.
You might want to look into using JavaScript's Built in
Date functionality.Code Snippets
switch (true) {
case (interval == 7):
//weekly
//Push in the selected dates in the selected array.
for (var i = 0; i < between.length; i += 7) {
selected.push(between[i]);
}
break;
case (interval == 30):
//Monthly
//Push in the selected dates in the selected array.
for (var i = 0; i < between.length; i += 30) {
selected.push(between[i]);
}
break;
case (interval == 15):
//Bi-Monthly
//Push in the selected dates in the selected array.
for (var i = 1; i < between.length; i += 15) {
selected.push(between[i]);
}
break;
case (interval == 0):
//One time event
//Push in the selected dates in the selected array.
for (var i = 0; i < 1; i++) {
selected.push(between[i]);
}
break;
default:
return undefined;
}case (interval == 0):
//One time event
//Push in the selected dates in the selected array.
selected.push(between[0]);
break;switch (true) {
case (interval == 7):
//weekly
for (var i = 0; i < between.length; i += 7) {
selected.push(between[i]);
}
break;
case (interval == 30):
//Monthly
for (var i = 0; i < between.length; i += 30) {
selected.push(between[i]);
}
break;
case (interval == 15):
//Bi-Monthly
for (var i = 1; i < between.length; i += 15) {
selected.push(between[i]);
}
break;
case (interval == 0):
//One time event
selected.push(between[0]);
break;
default:
return undefined;
}if (interval == 7) {
for (var i = 0; i < between.length; i += 7) {
selected.push(between[i]);
}
} else if (interval == 15) {
for (var i = 0; i < between.length; i += 15) {
selected.push(between[i]);
}
} else if (interval == 30) {
for (var i = 0; i < between.length; i += 30) {
selected.push(between[i]);
}
} else if (interval == 0) {
selected.push(between[0]);
} else {
return undefined;
}function GetRecurringDates(interval) {
if (interval != 0 || interval != 7 || interval != 15 || interval != 30) {
return undefined;
}
for (var i = 0; i < between.length; i += interval) {
selected.push(between[i]);
}
}Context
StackExchange Code Review Q#77085, answer score: 5
Revisions (0)
No revisions yet.