Schedule class (exercise for test driven development)

Question

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<DAY_OF_WEEK> 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("Date before limit");
 if (dateStop != null && dateToCheck.after(dateStop))
 throw new IllegalArgumentException("Date after limit");
}
public void checkMonthRepetition(Date dateToCheck) throws IllegalArgumentException{
 Calendar calendar = Calendar.getInstance();
 calendar.setTime(dateToCheck);
 int dayOfWeekToValidate = calendar.get(Calendar.DAY_OF_MONTH);
 calendar = Calendar.getInstance();
 calendar.setTime(dateStart);
 int dayOfWeek = calendar.get(Calendar.DAY_OF_MONTH);
 if (dayOfWeek != dayOfWeekToValidate){
 throw new IllegalArgumentException("Not same day of month.");
 }
}
private void checkWeekRepetition(Date dateToCheck) throws IllegalArgumentException{
 DAY_OF_WEEK day = evaluateDayOfWeek(dateToCheck);
 if (!daysOfWeek.contains(day))
 throw new IllegalArgumentException("No repetition in this day of week.");
}
private DAY_OF_WEEK evaluateDayOfWeek(Date date){
 Calendar calendar = Calendar.getInstance();
 calendar.setTime(date);
 int dayOfWeek = calendar.get(Calendar.DAY_OF_WEEK);
 switch (dayOfWeek){
 case Calendar.MONDAY: return DAY_OF_WEEK.MONDAY;
 case Calendar.TUESDAY: return DAY_OF_WEEK.TUESDAY;
 case Calendar.WEDNESDAY: return DAY_OF_WEEK.WEDNESDAY;
 case Calendar.THURSDAY: return DAY_OF_WEEK.THURSDAY;
 case Calendar.FRIDAY: return DAY_OF_WEEK.FRIDAY;
 case Calendar.SATURDAY: return DAY_OF_WEEK.SATURDAY;
 case Calendar.SUNDAY: return DAY_OF_WEEK.SUNDAY;
 default: throw new IllegalArgumentException("Unexpected value.");
 }
}
public Date getDateStop() {
 return dateStop;
}
public Date getDateStart() {
 return dateStart;
}
public void setRepetitionType(REPETITION repetitionType) {
 this.repetitionType = repetitionType;
}
public void addDayOfWeek(DAY_OF_WEEK day) {
 daysOfWeek.add(day);
}
public void removeDayOfWeek(DAY_OF_WEEK day){
 daysOfWeek.remove(day);
}
}

This is the test class (Junit4):

public class ScheduleTest {
Date mon = new Date(2014, 0, 5);
Date tue = new Date(2014, 0, 6);
Date wed = new Date(2014, 0, 7);
Date thu = new Date(2014, 0, 8);
Date fri = new Date(2014, 0, 9);
Date sat = new Date(2014, 0, 10);
Date sun = new Date(2014, 0, 11);
Date firstOf2014 = new Date(2014, 0, 1);
Date lastOf2014 = new Date(2014, 11, 31);
Date today = GregorianCalendar.getInstance().getTime();
Date tomorrow = new Date(today.getTime() + (1000L * 3600 * 24));
Date nextMonth = new Date(today.getTime() + (1000L * 3600 * 24 * 31));
Date prevMonth = new Date(today.getTime() - (1000L * 3600 * 24 * 31));
Schedule scheduleTodayTomorrow = new Schedule(today, tomorrow);
@Test
public void testDateStartBeforeDateStop(){
 try{
 scheduleTodayTomorrow = new Schedule(tomorrow, today);
 Assert.fail("Should throw IllegalArgumentException when dateStart after dateStop.");
 }
 catch (IllegalArgumentException ex){
 //OK
 }
}
@Test
public void testDateStopCanBeNull(){
 Schedule schedule = new Schedule(today, null);
}
@Test
public void testDefensiveCopy(){
 long todayTime = today.getTime();
 today.setTime(1000);
 Assert.assertEquals(todayTime, scheduleTodayTomorrow.getDateStart().getTime());
 long tomorrowTime = tomorrow.getTime();
 tomorrow.setTime(1000);
 Assert.assertEquals(tomorrowTime, scheduleTodayTomorrow.getDateStop().getTime());
}
@Test
public void testDateValidity(){
 Assert.assertTrue(scheduleTodayTomorrow.checkDateValid(today));
 Assert.assertTrue(scheduleTodayTomorrow.checkDateValid(tomorrow));
 Assert.assertFalse(scheduleTodayTomorrow.checkDateValid(prevMonth));
 Assert.assertFalse(scheduleTodayTomorrow.checkDateValid(nextMonth));
 Date inDate = new Date(today.getTime() + 1000L);
 Assert.assertTrue(scheduleTodayTomorrow.checkDateValid(inDate));
}
@Test
public void testDailyRepetition(){
 Schedule schedule = new Schedule(today, nextMonth);
 schedule.setRepetitionType(Schedule.REPETITION.DAILY);
 Assert.assertTrue(schedule.checkDateValid(today));
 Assert.assertTrue(schedule.checkDateValid(tomorrow));
 Assert.assertTrue(schedule.checkDateValid(nextMonth));
}
@Test
public void testMonthRepetition(){
 Schedule schedule = new Schedule(firstOf2014, lastOf2014);
 schedule.setRepetitionType(Schedule.REPETITION.MONTHLY);
 // As the scheduleTodayTomorrow starts on 1/1, then every first day of months should be valid
 for (int i=1; i<=11; i++){
 Date date = new Date(2014, i, 1);
 Assert.assertTrue("Valid date with month " + i + " treated as invalid.", schedule.checkDateValid(date));
 }
 // Every other days of every month are invalid
 for (int i=1; i<=11; i++){
 Date date = new Date(2014, i, 2);
 Assert.assertFalse("Invalid date with month " + i + " treated as valid.", schedule.checkDateValid(date));
 }
}
@Test
public void testWeekRepetitionNoValidDateAsDefault(){
 Schedule schedule = new Schedule(firstOf2014, lastOf2014);
 schedule.setRepetitionType(Schedule.REPETITION.WEEKLY);
 Assert.assertFalse(schedule.checkDateValid(mon));
 Assert.assertFalse(schedule.checkDateValid(tue));
 Assert.assertFalse(schedule.checkDateValid(wed));
 Assert.assertFalse(schedule.checkDateValid(thu));
 Assert.assertFalse(schedule.checkDateValid(fri));
 Assert.assertFalse(schedule.checkDateValid(sat));
 Assert.assertFalse(schedule.checkDateValid(sun));
}
@Test
public void testWeekRepetitionAddMonday(){
 Schedule schedule = new Schedule(firstOf2014, lastOf2014);
 schedule.setRepetitionType(Schedule.REPETITION.WEEKLY);
 schedule.addDayOfWeek(Schedule.DAY_OF_WEEK.MONDAY);
 Assert.assertTrue(schedule.checkDateValid(mon));
 Assert.assertFalse(schedule.checkDateValid(tue));
 Assert.assertFalse(schedule.checkDateValid(wed));
 Assert.assertFalse(schedule.checkDateValid(thu));
 Assert.assertFalse(schedule.checkDateValid(fri));
 Assert.assertFalse(schedule.checkDateValid(sat));
 Assert.assertFalse(schedule.checkDateValid(sun));
}
@Test
public void testWeekRepetitionAddTuesday(){
 Schedule schedule = new Schedule(firstOf2014, lastOf2014);
 schedule.setRepetitionType(Schedule.REPETITION.WEEKLY);
 schedule.addDayOfWeek(Schedule.DAY_OF_WEEK.TUESDAY);
 Assert.assertTrue(schedule.checkDateValid(tue));
 Assert.assertFalse(schedule.checkDateValid(mon));
 Assert.assertFalse(schedule.checkDateValid(wed));
 Assert.assertFalse(schedule.checkDateValid(thu));
 Assert.assertFalse(schedule.checkDateValid(fri));
 Assert.assertFalse(schedule.checkDateValid(sat));
 Assert.assertFalse(schedule.checkDateValid(sun));
}
@Test
public void testWeekRepetitionAddWednesday(){
 Schedule schedule = new Schedule(firstOf2014, lastOf2014);
 schedule.setRepetitionType(Schedule.REPETITION.WEEKLY);
 schedule.addDayOfWeek(Schedule.DAY_OF_WEEK.WEDNESDAY);
 Assert.assertTrue(schedule.checkDateValid(wed));
 Assert.assertFalse(schedule.checkDateValid(mon));
 Assert.assertFalse(schedule.checkDateValid(tue));
 Assert.assertFalse(schedule.checkDateValid(thu));
 Assert.assertFalse(schedule.checkDateValid(fri));
 Assert.assertFalse(schedule.checkDateValid(sat));
 Assert.assertFalse(schedule.checkDateValid(sun));
}
@Test
public void testWeekRepetitionAddThursday(){
 Schedule schedule = new Schedule(firstOf2014, lastOf2014);
 schedule.setRepetitionType(Schedule.REPETITION.WEEKLY);
 schedule.addDayOfWeek(Schedule.DAY_OF_WEEK.THURSDAY);
 Assert.assertTrue(schedule.checkDateValid(thu));
 Assert.assertFalse(schedule.checkDateValid(mon));
 Assert.assertFalse(schedule.checkDateValid(tue));
 Assert.assertFalse(schedule.checkDateValid(wed));
 Assert.assertFalse(schedule.checkDateValid(fri));
 Assert.assertFalse(schedule.checkDateValid(sat));
 Assert.assertFalse(schedule.checkDateValid(sun));
}
@Test
public void testWeekRepetitionAddFriday(){
 Schedule schedule = new Schedule(firstOf2014, lastOf2014);
 schedule.setRepetitionType(Schedule.REPETITION.WEEKLY);
 schedule.addDayOfWeek(Schedule.DAY_OF_WEEK.FRIDAY);
 Assert.assertTrue(schedule.checkDateValid(fri));
 Assert.assertFalse(schedule.checkDateValid(mon));
 Assert.assertFalse(schedule.checkDateValid(tue));
 Assert.assertFalse(schedule.checkDateValid(wed));
 Assert.assertFalse(schedule.checkDateValid(thu));
 Assert.assertFalse(schedule.checkDateValid(sat));
 Assert.assertFalse(schedule.checkDateValid(sun));
}
@Test
public void testWeekRepetitionAddSaturday(){
 Schedule schedule = new Schedule(firstOf2014, lastOf2014);
 schedule.setRepetitionType(Schedule.REPETITION.WEEKLY);
 schedule.addDayOfWeek(Schedule.DAY_OF_WEEK.SATURDAY);
 Assert.assertTrue(schedule.checkDateValid(sat));
 Assert.assertFalse(schedule.checkDateValid(mon));
 Assert.assertFalse(schedule.checkDateValid(tue));
 Assert.assertFalse(schedule.checkDateValid(wed));
 Assert.assertFalse(schedule.checkDateValid(thu));
 Assert.assertFalse(schedule.checkDateValid(fri));
 Assert.assertFalse(schedule.checkDateValid(sun));
}
@Test
public void testWeekRepetitionAddSunday(){
 Schedule schedule = new Schedule(firstOf2014, lastOf2014);
 schedule.setRepetitionType(Schedule.REPETITION.WEEKLY);
 schedule.addDayOfWeek(Schedule.DAY_OF_WEEK.SUNDAY);
 Assert.assertTrue(schedule.checkDateValid(sun));
 Assert.assertFalse(schedule.checkDateValid(mon));
 Assert.assertFalse(schedule.checkDateValid(tue));
 Assert.assertFalse(schedule.checkDateValid(wed));
 Assert.assertFalse(schedule.checkDateValid(thu));
 Assert.assertFalse(schedule.checkDateValid(fri));
 Assert.assertFalse(schedule.checkDateValid(sat));
}
@Test
public void testWeekRepetitionAddMultipleDays(){
 Schedule schedule = new Schedule(firstOf2014, lastOf2014);
 schedule.setRepetitionType(Schedule.REPETITION.WEEKLY);
 schedule.addDayOfWeek(Schedule.DAY_OF_WEEK.MONDAY);
 schedule.addDayOfWeek(Schedule.DAY_OF_WEEK.WEDNESDAY);
 schedule.addDayOfWeek(Schedule.DAY_OF_WEEK.FRIDAY);
 Assert.assertTrue(schedule.checkDateValid(mon));
 Assert.assertTrue(schedule.checkDateValid(wed));
 Assert.assertTrue(schedule.checkDateValid(fri));
 Assert.assertFalse(schedule.checkDateValid(tue));
 Assert.assertFalse(schedule.checkDateValid(thu));
 Assert.assertFalse(schedule.checkDateValid(sat));
 Assert.assertFalse(schedule.checkDateValid(sun));
}
@Test
public void testWeekRepetitionAddAndRemoveDays(){
 Schedule schedule = new Schedule(firstOf2014, lastOf2014);
 schedule.setRepetitionType(Schedule.REPETITION.WEEKLY);
 schedule.addDayOfWeek(Schedule.DAY_OF_WEEK.MONDAY);
 schedule.addDayOfWeek(Schedule.DAY_OF_WEEK.WEDNESDAY);
 schedule.addDayOfWeek(Schedule.DAY_OF_WEEK.FRIDAY);
 Assert.assertTrue(schedule.checkDateValid(mon));
 Assert.assertTrue(schedule.checkDateValid(wed));
 Assert.assertTrue(schedule.checkDateValid(fri));
 schedule.removeDayOfWeek(Schedule.DAY_OF_WEEK.MONDAY);
 Assert.assertFalse(schedule.checkDateValid(mon));
 Assert.assertTrue(schedule.checkDateValid(wed));
 Assert.assertTrue(schedule.checkDateValid(fri));
 schedule.removeDayOfWeek(Schedule.DAY_OF_WEEK.WEDNESDAY);
 Assert.assertFalse(schedule.checkDateValid(mon));
 Assert.assertFalse(schedule.checkDateValid(wed));
 Assert.assertTrue(schedule.checkDateValid(fri));
 schedule.removeDayOfWeek(Schedule.DAY_OF_WEEK.FRIDAY);
 Assert.assertFalse(schedule.checkDateValid(mon));
 Assert.assertFalse(schedule.checkDateValid(wed));
 Assert.assertFalse(schedule.checkDateValid(fri));
}
}

Answer 1

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)...

• holap: Thank you for suggestion about name. Actually I'm not very sutisfy with the names I choosed (english is not my mother language, have to improve this). Don't like constant at all. They where needed when enum didn't exist. Now I prefer use enum for the check that compiler could do. I choosed exception juat to stop the flow of method. Otherwise I should check the return value of every function and return if it's false. Don't like it, this way I can avoid it. Amazing suggestion about the api.
• holap: About checkMonthRepetition and checkWeekRepetition, now I can see your point (thanks to Simon). Yes that's should be better. I made it because I thought there could be other conditions to test after that. I actually should cange it.
• holap: Changed the name of checkDateValid to checkRepetitionRequiredAtDate and removed the exception, but would improve the style of that function.
• André Caron: Nice post overall, but I don't find your snippet on exceptions very helpful: "Exceptions should be used in case of, well, exceptions. Here they are used for completely expected values." Using a term as its own definition doesn't add anything. And besides, how could you raise an exception for invalid values, which are (obviously) expected since they are known up front and you are testing for them?
• Uri Agassi: @AndréCaron - thanks for your input - I've reworded the section according to it

Answer 2

I would strongly reccomend that you use the java.time package instead of the java.util classes, if that is at all possible.

---

One thing you could improve involves the improperly-named DAY_OF_WEEK and REPETITION enums. An enum is a type of class, and should be named using the convention for naming classes, not that for static final constants. Call them DayOfWeek and Repetition instead.

• Simon Forsberg: It should be emphasized that java.time package is for Java 8. But yes, it's a good suggestion to use it. Java 8 is awesome :) +1
• holap: I decided to use java 7 and avoid Joda library. Thank for names, still have to remember all the java naming convention.
• David Harkness: @holap JodaTime or java.time will save your sanity.

Answer 3

• private final Your fields dateStart, dateStop and the others should be marked as private final. Other classes don't need direct access to them (and normally shouldn't have direct access even if they needed it), and as they never change once the class has been initialized it's good practice to mark them as final.

• I'm not a big fan of the lack of line breaks in your switch statements. Also, by returning boolean as has been suggested in other answers, you can simplify it to: (Note the improved formatting)

  switch (repetitionType) {
   case WEEKLY: 
   return checkWeekRepetition(dateToCheck);
   case MONTHLY: 
   return checkMonthRepetition(dateToCheck);
  }
  

  I also removed the if (repetitionType != null) check as that's not needed, if it would be null the default case would apply, and as your default case was just empty there's no need in having it at all.

• catch (IllegalArgumentException ex) {

  You catch the IllegalArgumentException to use it as flow-control by returning false if it occurs. Don't do that. Don't use Exceptions for flow-control in the first place, and I also have to point out that some exceptions are not meant to be caught. That includes IllegalArgumentException See another answer of mine here regarding such exceptions.

• holap: Thank you. You made the argument of Uri more clear. Made some changes, but don't like a lot control flow of checkRepetitionRequiredAtDate (I renamed the function).

Answer 4

As far as I see nobody reviewed the tests, so here are a few notes about that:

1. For this:

   @Test
   public void testDateStartBeforeDateStop(){
    try{
    scheduleTodayTomorrow = new Schedule(tomorrow, today);
    Assert.fail("Should throw IllegalArgumentException when dateStart after dateStop.");
    }
    catch (IllegalArgumentException ex){
    //OK
    }
   }
   

   could be changed to

   @Test(expected = IllegalArgumentException.class)
   public void testDateStartBeforeDateStop() {
    new Schedule(tomorrow, today);
   }
   

   It's the same. If you want to test the message of the exception you need the try-catch but if it doesn't matter this one is simpler.

2. This constructor is deprecated:

   Date mon = new Date(2014, 0, 5);
   

   I'd create a helper method with a similar signature which calls Calendar.set as it is suggested by the javadoc of Date.

3. The Date fields could be private.

   (Should I always use the private access modifier for class fields?; Item 13 of Effective Java 2nd Edition: Minimize the accessibility of classes and members.)

4. The schedule variable is unused here:

   @Test
   public void testDateStopCanBeNull(){
    Schedule schedule = new Schedule(today, null);
   }
   

   The following is the same:

   @Test
   public void testDateStopCanBeNull() {
    new Schedule(today, null);
   }
   

5. If you check the code with a code coverage tool (I'm using Eclipse with EclEmma) you can find a few missed branches which the tests don't cover.

6. I'd use static import for assert methods to remove some clutter. So, this

   Assert.assertTrue(schedule.checkDateValid(mon));
   

   could be this:

   assertTrue(schedule.checkDateValid(mon));
   

7. Instead of constants like these:

   Date mon = new Date(2014, 0, 5);
   Date tue = new Date(2014, 0, 6);
   

   I would use creation methods (createMondayDate, etc.) to avoid accidental modification of the internal time of the Date object.

8. The following field is used by only two test methods:

   Schedule scheduleTodayTomorrow = new Schedule(today, tomorrow);
   

   I would instantiate it inside those methods to increase defect localization. If the constructor throws an exception for those parameters all test will fail which makes debugging a little bit harder.

9. This test tests two things:

   @Test
   public void testDefensiveCopy(){
    long todayTime = today.getTime();
    today.setTime(1000);
    Assert.assertEquals(todayTime, scheduleTodayTomorrow.getDateStart().getTime());
    long tomorrowTime = tomorrow.getTime();
    tomorrow.setTime(1000);
    Assert.assertEquals(tomorrowTime, scheduleTodayTomorrow.getDateStop().getTime());
   }
   

   It could be split up to two separate methods: testStartDateDefensiveCopy, testStopDateDefensiveCopy.

• David Harkness: This seems the best answer to add "Don't prefix your test method names with test when you use the @Test annotation.
• holap: Thank you. Very usefull tips. Intellij community didn't complain about branches, I will try with Eclipse.