Schedule class (exercise for test driven development)

Schedule is a simple class that could be used in program to manage when a task should be repeated (a todo list, for example). Its constructor requires a start date and an optional end date, then type of repetition (daily, weekly or monthly) has to be set. If weekly is chosen, then the days of weeks when repetition occurs has to be set too. This is a simple exercise I made to practice test driven development. All test passes, so I guess it works as expected. Any suggestion about how to improve tests will be very appreciated (but any other are welcome too).

This is the class:

```
public class Schedule {
public enum REPETITION {
NONE, DAILY, WEEKLY, MONTHLY
}

public enum DAY_OF_WEEK {
MONDAY, TUESDAY, WEDNESDAY, THURSDAY, FRIDAY, SATURDAY, SUNDAY
}

Date dateStart;
Date dateStop;
REPETITION repetitionType = REPETITION.NONE;
EnumSet daysOfWeek = EnumSet.noneOf(DAY_OF_WEEK.class);

public Schedule(Date dateStart, Date dateStop){
if (dateStop != null){
if (dateStart.after(dateStop))
throw new IllegalArgumentException("Date start after date stop.");

this.dateStop = new Date(dateStop.getTime());
}

this.dateStart = new Date(dateStart.getTime());
}

/**
*
* @param dateToCheck
* @return true if repetition is expected int the day to check
*/
public boolean checkDateValid(Date dateToCheck){
if (dateToCheck == null)
return false;

try {
checkDateBetweenLimits(dateToCheck);

if (repetitionType != null){
switch (repetitionType){
case WEEKLY: checkWeekRepetition(dateToCheck); break;
case MONTHLY: checkMonthRepetition(dateToCheck); break;
default:
}
}
}
catch (IllegalArgumentException ex){
return false;
}

return true;
}

public void checkDateBetweenLimits(Date dateToCheck) throws IllegalArgumentException{
if (dateToCheck.before(dateStart))
throw new IllegalArgumentException("Da

What's in a name?

Class names should be nouns, and although Schedule could be a noun, it looks like you chose the verb name. I think maybe 'Task' will be a better name for it?

Your main method is called checkDateValid(), what does it mean? All dates passed to the method are valid... valid for what? Give the method a name which gives the context of the instance which tests it.

The documentation above the method says

* @return true if repetition is expected int the day to check

And what happens if repetition is not expected? This is actually not defined in your description as well... By looking in your code it means that if there is no repetition, anytime between start date and end date (or forever) is considered valid... Maybe a better name for the method be isRelevantFor()?

When enums don't matter

You have declared two enums - REPETITION and DAY_OF_WEEK. One observation - naming conventions for enums is CamelCase, so the names should be Repetition and DayOfWeek.

But, look at what you do with DAY_OF_WEEK - you translate it (not in a very DRY way) to Calendar.DAY_OF_WEEK, you should consider to drop it all together and use the Calendar constants from the start...

What exceptions are for?

checkMonthRepetition and checkWeekRepetition throw IllegalArgumentException. Why? Again, all dates are legal.

Exceptions should be used in case where something prevented the method from completing its promise to its caller. For example - parseInt() would throw an exception when the input string is not made of integers - it simply can't parse it to a valid integer. The same way that a readFile() will throw an exception when the file was not found where expected.

Here they are used for completely expected values - the input date is either a match to the indicated repetition dates, or it isn't.

Exceptions break the flow of the method, and need to be handled. They are also very inefficient at runtime.

These methods should simply return boolean, just like their caller.

Simplify you API according to use-cases

To set a weekly repetition, you expect the user to call two methods - setRepetitionType() and then remember to call addDayOfWeek. This is far from optimal to your prospective user, and very hard to enforce statically. The reason you chose this way is because that is the way your class is arranged internally.

If you think about the API from the outside, maybe a better set of APIs would be setMonthlyRepetition(), and setWeeklyRepetition(DAY_OF_WEEK... days). This way, the user will set the relevant days in the same call where he sets the repetition type. It would also make it easier to expand your API in the future to setMonthlyRepetition(int... daysOfTheMonth)...

StackExchange Code Review Q#46282, answer score: 11