Week 5: Test Driven Development #10
Conversation
walkthroughs/week-5-tdd/project/src/main/java/com/google/sps/FindMeetingQuery.java
Show resolved
Hide resolved
|
|
||
| // Returns a TimeRange >= "duration" that overlaps 2 time ranges | ||
| private TimeRange findOverlap(TimeRange time1, TimeRange time2, long duration) { | ||
| // Case 1: |+++| |---| |
There was a problem hiding this comment.
Very readable comments here and in test file!
Another way I think might be easier to implement is:
start = max(start1, start2)
end = min(end1, end2)
check start < end
There was a problem hiding this comment.
Thanks! I actually found a much easier way to incorporate optional attendees and ended up deleting this whole function!
| nextTime = busyTimes.get(i); | ||
|
|
||
| // If the times overlap, create a new TimeRange that merges them and set "thisTime" to it | ||
| if (thisTime.overlaps(nextTime)) { |
There was a problem hiding this comment.
I feel it would be more extensible if we could create a set of utils:
- findOverlap() to find intersection of times
- Extract logic here to find union of times
- Change getOpenTimes() to find complement of times (so that it makes sense when we have a list of open times, we call it for a list of busy times)
There was a problem hiding this comment.
What part are you referring to for finding the union of times?
I moved the code for merging the overlapped busy times into a new helper function, "mergeOverlappedTimes()"
And I'm not sure if it makes sense to change getOpenTimes() to find the complement of times because getOpenTimes() doesn't find the exact complement. It only adds the open times if they can fit the meeting duration ("duration" parameter). I'm not sure what this parameter would do if we have a list of open times and call the function for a list of busy times. What do you think?
There was a problem hiding this comment.
It's just line 107 and 108 I was referring to for finding the union of times. Don't worry about this since we are not finding the intersection now and it's not necessary to create the utils.
For getOpenTimes(), I only meant to change the name, so that it makes sense when we have a list of open times, we call it with duration=0 for a list of busy times. Let's leave it here and only do it if we really need to reuse this function.
| } | ||
|
|
||
| @Test | ||
| public void optionalAttendeeCannotAttend() { |
There was a problem hiding this comment.
Do these tests pass now? I thought there are TODOs?
There was a problem hiding this comment.
You're right. I added 5 tests for the optional attendees before writing the code so they have not been tested yet. Should I be marking which tests pass/ fail?
There was a problem hiding this comment.
Yeah we don't add test cases that are not implemented yet. We won't be able to submit failing tests due to Google's TapPresubmit checks.
I guess they all pass today?
There was a problem hiding this comment.
Oh ok. Yes these all pass now.
| } | ||
|
|
||
| // Return the whole day if there are no busy times | ||
| return Arrays.asList(TimeRange.WHOLE_DAY); |
There was a problem hiding this comment.
I think we normally do early return for edge cases?
There was a problem hiding this comment.
Oh yeah I'm not sure why I had it like that. Fixed it!
| private List<TimeRange> mergeOverlappedTimes(List<TimeRange> busyTimes) { | ||
|
|
||
| List<TimeRange> busyTimesFinal = new ArrayList<TimeRange>(); | ||
| TimeRange thisTime = busyTimes.get(0); |
There was a problem hiding this comment.
Will there be an error if called with an empty list?
There was a problem hiding this comment.
Good catch. I added an early return
| nextTime = busyTimes.get(i); | ||
|
|
||
| // If the times overlap, create a new TimeRange that merges them and set "thisTime" to it | ||
| if (thisTime.overlaps(nextTime)) { |
There was a problem hiding this comment.
It's just line 107 and 108 I was referring to for finding the union of times. Don't worry about this since we are not finding the intersection now and it's not necessary to create the utils.
For getOpenTimes(), I only meant to change the name, so that it makes sense when we have a list of open times, we call it with duration=0 for a list of busy times. Let's leave it here and only do it if we really need to reuse this function.
| } | ||
|
|
||
| @Test | ||
| public void optionalAttendeeCannotAttend() { |
There was a problem hiding this comment.
Yeah we don't add test cases that are not implemented yet. We won't be able to submit failing tests due to Google's TapPresubmit checks.
I guess they all pass today?
2 participants
This week, we are asked to implement a function in a Calendar project. It basically finds all available times for a list of people that a meeting can be scheduled at.
The first milestone was to implement this function for mandatory attendees.
Next, we were asked to write 5 JUnit tests that account for optional attendees. If no times for mandatory and optional attendees are available, then just return the times for the mandatory attendees.
Finally, we need to update the function to account for the optional attendee functionality.