Added tests for optional attendees and modified query to handle optional attendees #22
Conversation
| if (request.getOptionalAttendees().isEmpty()) { | ||
| attendees.removeAll(request.getOptionalAttendees()); | ||
| } |
There was a problem hiding this comment.
This seems unnecessary. You would be removing nothing?
| if (request.getAttendees().isEmpty()) { | ||
| attendees.removeAll(request.getAttendees()); | ||
| } |
| if (queryHelper(eventCollection, request, attendees).isEmpty()) { | ||
| attendees.removeAll(request.getOptionalAttendees()); | ||
| return queryHelper(eventCollection, request, attendees); | ||
| } | ||
| return queryHelper(eventCollection, request, attendees); |
There was a problem hiding this comment.
You should store the result of each of these calls to queryHelper rather than re-calling it. You always end up calling it one more time than necessary, which wastes time given it is a somewhat complex function.
| return queryHelper(eventCollection, request, attendees); | ||
| } | ||
|
|
||
| private static Collection<TimeRange> queryHelper(Collection<Event> groupOfEvents, MeetingRequest request, Collection<String> attendees) { |
There was a problem hiding this comment.
You can probably just call the first parameter "events." Using the plural name makes clear that it's some sort of collection/list, so the groupOf prefix is unnecessary.
| private static boolean rangeLessThanDuration(TimeRange range, long meetingDuration) { | ||
| return (range.duration() >= meetingDuration); | ||
| } | ||
|
|
| List<String> meetingAttendees = new ArrayList<String>(request.getAttendees()); | ||
| if (events.isEmpty() || meetingAttendees.isEmpty()) { | ||
| // No events | ||
| List<Event> events = new ArrayList<Event>(groupOfEvents); |
There was a problem hiding this comment.
Why are you doing this? Is it because you're accepting "groupOfEvents" as a "Collection"? Consider updating the function signature to accept a better collection type.
| return queryHelper(eventCollection, request, attendees); | ||
| } | ||
|
|
||
| private static Collection<TimeRange> queryHelper(Collection<Event> groupOfEvents, MeetingRequest request, Collection<String> attendees) { |
There was a problem hiding this comment.
I made this note below, but I'll echo it here.
Collection is a really weird argument type to accept. It makes no guarantees about the performance of operations on it, which is not generally what you want when you know you'll iterate through them.
Consider accepting List<> or ArrayList<>.
| } | ||
|
|
||
| if (start < latestEventEnd) { | ||
| if (start <= latestEventEnd) { |
There was a problem hiding this comment.
Nit: This = here isn't doing anything since you're detecting == above.
| public Collection<TimeRange> query(Collection<Event> eventCollection, MeetingRequest request) { | ||
|
|
||
| public Collection<TimeRange> query(Collection<Event> eventCollection, MeetingRequest request) { | ||
| Collection<String> attendees = new ArrayList<String>(); |
There was a problem hiding this comment.
Ah, heres the collection.
Yeah, I said before to use generic types, but Collection is too generic, lol. List is probably what you want.
No description provided.