Finish find meeting algorithm and add test cases #6
Conversation
walkthroughs/week-5-tdd/project/src/test/java/com/google/sps/FindMeetingQueryTest.java
Outdated
Show resolved
Hide resolved
walkthroughs/week-5-tdd/project/src/test/java/com/google/sps/FindMeetingQueryTest.java
Outdated
Show resolved
Hide resolved
walkthroughs/week-5-tdd/project/src/test/java/com/google/sps/FindMeetingQueryTest.java
Outdated
Show resolved
Hide resolved
walkthroughs/week-5-tdd/project/src/main/java/com/google/sps/FindMeetingQuery.java
Outdated
Show resolved
Hide resolved
walkthroughs/week-5-tdd/project/src/main/java/com/google/sps/FindMeetingQuery.java
Outdated
Show resolved
Hide resolved
|
|
||
| // Save the result in timeSlotsForRequired | ||
| // in case no time slot can be found when optional attendees are included | ||
| List<TimeRange> timeSlotsForRequired = new ArrayList<TimeRange>(timeSlots); |
There was a problem hiding this comment.
Is it necessary to convert timeSlots to an ArrayList?
There was a problem hiding this comment.
I am saving a copy of timeSlots to variable timeSlotsForRequired (in the case where there is no time slot available for optional attendees, I can just return timeSlotsForRequired).
| List<TimeRange> timeSlotsForRequired = new ArrayList<TimeRange>(timeSlots); | ||
|
|
||
| // Check for the availability of optional attendees | ||
| timeSlots = queryForEvent(events, timeSlots, request.getOptionalAttendees()); |
There was a problem hiding this comment.
Does "timeSlots" include slots for required attendees, or only optional attendees?
There was a problem hiding this comment.
The variable timeSlots is first modified to fit all the required attendees' schedules, and the result is saved in timeSlotsForRequired. Then it goes on to check all the optional attendees, so after line 43, timeSlots include slots for both required and optional attendees. Do you think it is better to change the variable name to something like timeSlotsForAll? (I think the only problem with this is that making a copy of timeSlotsForAll and naming it timeSlotsForRequired does not make a lot of sense) Or do you think it is better to just add some comments here?
walkthroughs/week-5-tdd/project/src/main/java/com/google/sps/FindMeetingQuery.java
Outdated
Show resolved
Hide resolved
walkthroughs/week-5-tdd/project/src/main/java/com/google/sps/FindMeetingQuery.java
Outdated
Show resolved
Hide resolved
| // Case C: |--C---| | ||
| // Event: |------| | ||
| // |-1-| | ||
| // Shorten time period from C to 1 |
There was a problem hiding this comment.
- Good use of ASCII art to clarify the algorithm's different scenarios, I like it!
- Do 'slot' and 'period' mean the same thing? If yes, I'd suggest standardizing for clarity.
- I like how one timeSlot is named "Event". To be complete, what would be the name of the other two timeslots (lines 83 and 85)?
There was a problem hiding this comment.
- Fixed. 3) I changed Case C and event to their corresponding variable name so it matches with the code better.
walkthroughs/week-5-tdd/project/src/main/java/com/google/sps/FindMeetingQuery.java
Outdated
Show resolved
Hide resolved
| // Have each person have different events. We should see three options because each person has | ||
| // split the restricted times. |
There was a problem hiding this comment.
What does "each person has split the restricted times" mean?
There was a problem hiding this comment.
Basically means the events each attendee has split the whole day into 3 options for the request. I've changed the wording so that it's less confusing.
walkthroughs/week-5-tdd/project/src/test/java/com/google/sps/FindMeetingQueryTest.java
Outdated
Show resolved
Hide resolved
| Collection<TimeRange> expected = Arrays.asList(); | ||
|
|
||
| Assert.assertEquals(expected, actual); | ||
| } |
There was a problem hiding this comment.
- Should we add a test for the case where no time slots are available?
- What other edge cases should be considered / are worth testing?
There was a problem hiding this comment.
Some other tests are provided in the file as starter code (when the attendees have overlapping/nested events, when there is no time slots available, when there is no attendee at all, etc.)
Complete algorithm in FindMeetingQuery.java and add more edge cases to FindMeetingQueryTest.java