HiveBrain v1.2.0
Get Started
← Back to all entries
patternjavascriptMinor

Recurring dates within a range

Submitted by: @import:stackexchange-codereview··
0
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

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.