Building Tempo with Rails, Part V
Posted by Brian Sam-Bodden
This installment was supposed to be about building the Dashboard page of the Tempo application. Instead, I'm addressing some bugs reported by readers.
Fixing Bugs TDD-style
An observant reader pointed out that one of the tests for the TimeEntry class broke when testing using a date range across a month boundary.
The offending code is shown below:
def split!
remaining_time_entries = []
if needs_splitting
# save the original end time
original_end = self.end
# first change the current TimeEntry to the end_of_day of the start day
self.end = self.start.change(:hour => 23, :min => 59, :sec => 59)
(self.start.day+1..original_end.day).each do |day|
time_entry = self.clone
time_entry.start = time_entry.start.change(:mday => day).beginning_of_day
if day == original_end.day
time_entry.end = original_end
else
time_entry.end = time_entry.start.change(:hour => 23, :min => 59, :sec => 59)
end
remaining_time_entries << time_entry
end
end
remaining_time_entries
end
Even though the code above was created in a TDD fashion and several of the tests created exercised the code, the tests failed to cover all of the possible cases.
The lesson here is that when dealing with date ranges, you should test across months and year boundaries. The code above only works when the two datetimes are in the same month and year which is a critical flaw. The code uses the day method to loop form the start day to the end day of the range. This will obviously break across months and of course across years.
One good thing about the test above was (as some might argue whether this is a good thing or not) is that I used a date range that started on the current date and time. In a continuous integration scenario this broken test would had revealed itself in the hourly build.
I've also made the same mistake (of using the day method) to check if a range of dates needed splitting, here's the original code:
def needs_splitting self.start.day < self.end.day end
First, let's prove that the code is indeed broken. To accomplish this I've written a test that crosses a month boundary:
it "should know how to split a time entry across multiple days over a month boundary" do
time_entry = create_time_entry
time_entry.start = Time.local(2007,"nov",30,23,0,0)
time_entry.end = 5.hours.since(time_entry.start)
entries = time_entry.split!
entries.should_not be_empty
total = 0.0
entries.inject(0) {|total, e| total += e.total_hours}
total += time_entry.total_hours
total.should eql(5.0)
time_entry.needs_splitting.should_not be_true
end
The above test fails with the existing code. Now we can refactor the code to pass the test.
def needs_splitting
(self.start.year != self.end.year) ||
(self.start.month != self.end.month) ||
(self.start.day != self.end.day)
end
def split!
remaining_time_entries = []
if needs_splitting
# save the original end time
original_end = self.end
# first change the current TimeEntry to the end_of_day of the start day
self.end = end_of_day(self.start)
days = ((original_end - self.start) / 86400).ceil.to_i
(1..days).each do |day|
time_entry = self.clone
time_entry.start = time_entry.start.advance(:days => day).beginning_of_day
time_entry.end = time_entry.end.advance(:days => day)
if same_day(time_entry.end, original_end)
time_entry.end = time_entry.end.change(:hour => original_end.hour, :min => original_end.min, :sec => original_end.sec)
else
time_entry.end = end_of_day(time_entry.start)
end
remaining_time_entries << time_entry
end
end
remaining_time_entries
end
The needs_splitting method now checks the year, month and day. The split! method now finds the total number of whole days (plus one) in the datetime range. It then creates new TimeEntry(s) for each of the whole days (24 hours each) and a partial one of the last day on the range. To advance each one of the days I use the advance method (see Rails extensions to the Time class).
