Optional attendees #9
Conversation
| attendees.retainAll(event.getAttendees()); | ||
| // Attendees is only required attendees, allAttendees includes optional attendees | ||
| Set<String> requiredAttendees = new HashSet<>(request.getAttendees()); | ||
| Set<String> optionalAttendees = new HashSet<>(request.getOptionalAttendees()); |
There was a problem hiding this comment.
You may take this out of the loop so that you don't need redundant set creations.
| Set<String> optionalAttendees = new HashSet<>(request.getOptionalAttendees()); | ||
| Set<String> allAttendees = new HashSet<>(Sets.union(requiredAttendees, optionalAttendees)); | ||
|
|
||
| // I couldn't think of a way to factor this, I'm sorry Zu :'( |
There was a problem hiding this comment.
This is looks fine with me. Please remove this comment.
| List<TimeRange> requiredOpenings = new ArrayList<>(findOpenings(attendedMeetings, duration)); | ||
| List<TimeRange> allOpenings = new ArrayList<>(findOpenings(allAttendedMeetings, duration)); | ||
|
|
||
| return allOpenings.size() == 0 ? requiredOpenings : allOpenings; |
There was a problem hiding this comment.
Please add a good comment in the method header on what it returns ("if there is an opening for eveyone, return it, otherwise return the opening for the required").
In general, this is not the most recommended interface design, because the caller wouldn't (explicitly) know whether the returned openings are for everyone or for the required only. It is recommended to make the interface explicit about what it returns to minimize any potential error/bug. At minimum, it should be explicit in the comment.
There was a problem hiding this comment.
Made explicit in a comment. I definitely understand your concerns, and will revisit further if you'd like
| List<TimeRange> allAttendedMeetings = new ArrayList<>(); | ||
|
|
||
| // Attendees is only required attendees, allAttendees includes optional attendees | ||
| Set<String> requiredAttendees = new HashSet<>(request.getAttendees()); |
There was a problem hiding this comment.
You cannot take out requiredAttendees because you eventually changes them bellow (retainAll). That reminds me of a lesson that you want to make objects immutable whenever possible. Can you move the requiredAttendees back in and make optionalAttendees ImmutableSet?
Also try to find out other immutable containers throughout the code and change them to immutable.
2 participants
Calendar algorithm updated to consider optional attendees!