Return to Answer

• 1

• 30.3k
• 9
• 82
• 157

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.

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

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

1. 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);
 }

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

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

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

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

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