4
\$\begingroup\$

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;
            }
\$\endgroup\$
5
\$\begingroup\$

Both of the current answers address code style issues you have, but miss some significant bugs, and bad practices.

Bugs

  • weekly - this one is OK, actually

  • monthly - this one is a problem because you can possibly skip months entirely, or have multiple dates in a month.

    Consider an ugly year, day 0 happens to be January 1. Your monthly system will select January 1, January 31, and March 2. There you have two dates in Jan, and none in February.

  • bi-monthly (which, is actually semi-monthly - bi-monthly means every two months) - This code has two bugs. The first bug is the same as the monthly bug. You can have some months with 3 dates, and February may only have 1.

    The second bug is an inconsistency, for all the other systems, your loop starts at 0, and you select from the first entry in between. In this loop, though, you start from 1. This is inconsistent. It also means you may miss a period if the last date is supposed to be included...

  • daily - here the bug is that you only loop once, and you should include all the between dates.

Observations

In general, date processing is hard.

The daily, and weekly schedules are managable, but the monthly and semi-monthly schedules require more smarts than what you have coded so far. It also requires more of a specification than what you have given.

Should monthly be the first day of the month? Should it be the last? Should it be the 30th, except for Februaries where it should be the 28th, or 29th? Should it be the same day-of-month as today, but adjusted for february and other months if our day does not exist in those months?

For the semi-monthly system there's the same problem.

Your specification is too loose for us to go further with this implementation.

\$\endgroup\$
1
  • \$\begingroup\$ This is a pretty good feedback. I'm missing so much and wrote a buggy code ensures that I'm a beginner. From your observations I'll go with the option "same day-of-month as today" as base. What you can refer me to before going to write any code for this kind of recurrence? \$\endgroup\$
    – Superman
    Jan 10 '15 at 5:57
5
\$\begingroup\$

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.

\$\endgroup\$
1
  • \$\begingroup\$ you're Welcome, @Superman. \$\endgroup\$
    – Malachi
    Jan 9 '15 at 16:07
2
\$\begingroup\$

The first thing I notice, is your usage of switch (...) {...}. By convention, switch statements should be written like this:

switch (iterval) {
    case 1:
        // here, interval === 1
        break;
    case 7:
        // here, interval === 7
        break;
    // etc.
}

Technically, that is not the same as your code, because switch statements use strict comparision. You should always use strict comparision when possible, though. It's faster, since it doesn't do type conversion, and safer, for the same reason.


Inside your //One time event case, you use a loop for no reason, it will only run the body once. You can remove the loop, and change the code to:

case 0:
    //One time event
    //Push in the selected date  in the selected array.
    selected.push(between[i]);

In your first case statement, your indentation is incorrect, the break; statement is unindented, when it shouldn't be


The final code after making these modifications is below:

switch (interval) {
    case 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 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 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 0:
        //One time event
        //Push in the selected date in the selected array.
        selected.push(between[0]);
        break;
    default:
        return undefined;
    }
}
\$\endgroup\$
1
  • \$\begingroup\$ The way I'm finding the recurring dates is fine? And especially what about the bi-monthly recurrence? \$\endgroup\$
    – Superman
    Jan 9 '15 at 15:01

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.