Skip to content

Optional attendees #9

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
AlvieH wants to merge 4 commits into
base: master
Choose a base branch
from
Open

Optional attendees #9

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
5d5bfe8
optional attendees integrated
AlvieH Jun 18, 2020
1902dc8
style update
AlvieH Jun 19, 2020
f360fa4
style improvements
AlvieH Jun 22, 2020
35fab9c
top comment explains interface and return values
AlvieH Jun 22, 2020
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
5 changes: 5 additions & 0 deletions walkthroughs/week-5-tdd/project/pom.xml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -30,6 +30,11 @@
<version>2.8.6</version>
</dependency>

<dependency>
<groupId>com.google.collections</groupId>
<artifactId>google-collections</artifactId>
<version>1.0</version>
</dependency>

<dependency>
<groupId>junit</groupId>
Expand Down
85 changes: 59 additions & 26 deletions walkthroughs/week-5-tdd/project/src/main/java/com/google/sps/FindMeetingQuery.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -14,6 +14,7 @@

package com.google.sps;

import com.google.common.collect.Sets;
import java.util.Collection;
import java.util.Collections;
import java.util.HashSet;
Expand All @@ -23,6 +24,9 @@
import java.util.List;

public final class FindMeetingQuery {
// Returns available meetings for day given events and request. query will return a list of TimeRanges including optional attendees
// if there is availability with optional attendees, and will otherwise return a list of TimeRanges for only required attendees. Returns
// an empty List object if there is no availability.
public Collection<TimeRange> query(Collection<Event> events, MeetingRequest request) {
// In this case, long to int conversion is safe because duration can never exceed 2^32. Leaving duration as long
// leads to compile errors.
Expand All @@ -34,23 +38,73 @@ public Collection<TimeRange> query(Collection<Event> events, MeetingRequest requ
}

// Seperate relevant TimeRanges from events, put into ArrayList and sort by ascending meeting start time
// Optional attendees are handled by running two sets of queries in parallel: a query if we are including optional attendees,
// and a query if we are only considering required attendees. If the optional attendee query is empty, then that means that
// there are no valid times if we include optional attendees, and optional attendees are ignored.
List<TimeRange> attendedMeetings = new ArrayList<>();
List<TimeRange> allAttendedMeetings = new ArrayList<>();

// Attendees is only required attendees, allAttendees includes optional attendees
Set<String> requiredAttendees = new HashSet<>(request.getAttendees());
Copy link
Copy Markdown
Collaborator

@zkoogle zkoogle Jun 22, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Set<String> optionalAttendees = new HashSet<>(request.getOptionalAttendees());

for (Event event : events) {
// First check if the event in question contains people from request, add those meetings to attendedMeetings
Set<String> attendees = new HashSet<>(request.getAttendees());
attendees.retainAll(event.getAttendees());
Set<String> allAttendees = new HashSet<>(Sets.union(requiredAttendees, optionalAttendees));

requiredAttendees.retainAll(event.getAttendees());
allAttendees.retainAll(event.getAttendees());

if (!attendees.isEmpty()){
if (!requiredAttendees.isEmpty()){
attendedMeetings.add(event.getWhen());
}

if (!allAttendees.isEmpty()) {
allAttendedMeetings.add(event.getWhen());
}

}

// Sort attendedMeetings so that we can filter out all nested meetings in next step
Collections.sort(attendedMeetings, TimeRange.ORDER_BY_START);
Collections.sort(allAttendedMeetings, TimeRange.ORDER_BY_START);

// All openings represents the availabilities WITH optional attendees, requiredOpenings represents only required attendees.
// if allOpenings is empty, that means that there is a conflict with optional attendees and optional attendees should be ignored.
List<TimeRange> requiredOpenings = new ArrayList<>(findOpenings(attendedMeetings, duration));
List<TimeRange> allOpenings = new ArrayList<>(findOpenings(allAttendedMeetings, duration));

// As-is, the user will not be aware whether this function is returning allOpenings or requiredOpenings. As-is, the function
// simply returns whichever is available.
return allOpenings.size() == 0 ? requiredOpenings : allOpenings;
Copy link
Copy Markdown
Collaborator

@zkoogle zkoogle Jun 22, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Copy Markdown
Owner Author

@AlvieH AlvieH Jun 22, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Made explicit in a comment. I definitely understand your concerns, and will revisit further if you'd like

}

// Takes in sorted list of meetings and returns next available TimeRange given, or null if none available with current startTime
private TimeRange findNextTime(List<TimeRange> meetings, int startMeetingIndex, int duration) {
int startTime = meetings.get(startMeetingIndex).end();

List<TimeRange> validMeetings = new ArrayList<>();
// If this is the last meeting in the list and there's a meeting time that fits, return a meeting time.
if (startMeetingIndex == meetings.size() - 1) {
return (startTime + duration <= TimeRange.END_OF_DAY) ?
TimeRange.fromStartEnd(startTime, TimeRange.END_OF_DAY, true)
: null;
}

for (TimeRange meeting : attendedMeetings) {
// If next meeting's start time is farther away than duration,
// return a TimeRange from startTime -> the start of the next meeting.
if (meetings.get(startMeetingIndex + 1).start() - startTime >= duration) {
return TimeRange.fromStartEnd(startTime, meetings.get(startMeetingIndex + 1).start(), false);
}
else {
return null;
}
}

// Takes in sorted list of TimeRanges, filters out nested meetings, and then passes into findNextTime
private List<TimeRange> findOpenings(List<TimeRange> meetings, int duration) {
List<TimeRange> validMeetings = new ArrayList<>();

for (TimeRange meeting : meetings) {

int numMeetings = validMeetings.size();

Expand Down Expand Up @@ -85,25 +139,4 @@ public Collection<TimeRange> query(Collection<Event> events, MeetingRequest requ

return openings;
}

// Takes in sorted list of meetings and returns next available TimeRange given, or null if none available with current startTime
private TimeRange findNextTime(List<TimeRange> meetings, int startMeetingIndex, int duration) {
int startTime = meetings.get(startMeetingIndex).end();

// If this is the last meeting in the list and there's a meeting time that fits, return a meeting time.
if (startMeetingIndex == meetings.size() - 1) {
return (startTime + duration <= TimeRange.END_OF_DAY) ?
TimeRange.fromStartEnd(startTime, TimeRange.END_OF_DAY, true)
: null;
}

/* If next meeting's start time is farther away than duration,
* return a TimeRange from startTime -> the start of the next meeting. */
if (meetings.get(startMeetingIndex + 1).start() - startTime >= duration) {
return TimeRange.fromStartEnd(startTime, meetings.get(startMeetingIndex + 1).start(), false);
}
else {
return null;
}
}
}
142 changes: 142 additions & 0 deletions walkthroughs/week-5-tdd/project/src/test/java/com/google/sps/FindMeetingQueryTest.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -34,10 +34,12 @@ public final class FindMeetingQueryTest {
// Some people that we can use in our tests.
private static final String PERSON_A = "Person A";
private static final String PERSON_B = "Person B";
private static final String PERSON_C = "Person C";

// All dates are the first day of the year 2020.
private static final int TIME_0800AM = TimeRange.getTimeInMinutes(8, 0);
private static final int TIME_0830AM = TimeRange.getTimeInMinutes(8, 30);
private static final int TIME_0845AM = TimeRange.getTimeInMinutes(8, 45);
private static final int TIME_0900AM = TimeRange.getTimeInMinutes(9, 0);
private static final int TIME_0930AM = TimeRange.getTimeInMinutes(9, 30);
private static final int TIME_1000AM = TimeRange.getTimeInMinutes(10, 0);
Expand Down Expand Up @@ -270,4 +272,144 @@ public void notEnoughRoom() {

Assert.assertEquals(expected, actual);
}

@Test
public void optionalAttendeeNotAvailable() {
// Have each person have different events. We should see two options because each person has
// split the restricted times. Optional attendee C will not be considered because they are never
// available.
//
// Events : |--A--| |--B--|
// |-------------C---------------|
// Day : |-----------------------------|
// Options : |--1--| |--2--| |--3--|

Collection<Event> events = Arrays.asList(
new Event("Event 1", TimeRange.fromStartDuration(TIME_0800AM, DURATION_30_MINUTES),
Arrays.asList(PERSON_A)),
new Event("Event 2", TimeRange.fromStartDuration(TIME_0900AM, DURATION_30_MINUTES),
Arrays.asList(PERSON_B)),
new Event("Event 3", TimeRange.fromStartEnd(TimeRange.START_OF_DAY, TimeRange.END_OF_DAY, true),
Arrays.asList(PERSON_C)));

MeetingRequest request =
new MeetingRequest(Arrays.asList(PERSON_A, PERSON_B), DURATION_30_MINUTES);

request.addOptionalAttendee(PERSON_C);

Collection<TimeRange> actual = query.query(events, request);
Collection<TimeRange> expected =
Arrays.asList(TimeRange.fromStartEnd(TimeRange.START_OF_DAY, TIME_0800AM, false),
TimeRange.fromStartEnd(TIME_0830AM, TIME_0900AM, false),
TimeRange.fromStartEnd(TIME_0930AM, TimeRange.END_OF_DAY, true));

Assert.assertEquals(expected, actual);
}

@Test
public void optionalAttendeeIncluded() {
// Have each person have different events. We should see two options because each person has
// split the restricted times. Optional attendee Person C is available for some timeslots, but not all.
//
// Events : |--A--| |--B--|
// |--C--|
// Day : |-----------------------------|
// Options : |--1--| |--2--|

Collection<Event> events = Arrays.asList(
new Event("Event 1", TimeRange.fromStartDuration(TIME_0800AM, DURATION_30_MINUTES),
Arrays.asList(PERSON_A)),
new Event("Event 2", TimeRange.fromStartDuration(TIME_0900AM, DURATION_30_MINUTES),
Arrays.asList(PERSON_B)),
new Event("Event 3", TimeRange.fromStartDuration(TIME_0830AM, DURATION_30_MINUTES),
Arrays.asList(PERSON_C)));

MeetingRequest request =
new MeetingRequest(Arrays.asList(PERSON_A, PERSON_B), DURATION_30_MINUTES);
request.addOptionalAttendee(PERSON_C);

Collection<TimeRange> actual = query.query(events, request);
Collection<TimeRange> expected =
Arrays.asList(TimeRange.fromStartEnd(TimeRange.START_OF_DAY, TIME_0800AM, false),
TimeRange.fromStartEnd(TIME_0930AM, TimeRange.END_OF_DAY, true));

Assert.assertEquals(expected, actual);
}

@Test
public void justEnoughRoomIgnoreOptional() {
// Have one person, but make it so that there is just enough room at one point in the day to
// have the meeting if you ignore optional attendee B.
//
// Events : |--A--| |----A----|
// |-B|
// Day : |---------------------|
// Options : |-----|

Collection<Event> events = Arrays.asList(
new Event("Event 1", TimeRange.fromStartEnd(TimeRange.START_OF_DAY, TIME_0830AM, false),
Arrays.asList(PERSON_A)),
new Event("Event 2", TimeRange.fromStartEnd(TIME_0900AM, TimeRange.END_OF_DAY, true),
Arrays.asList(PERSON_A)),
new Event("Event 3", TimeRange.fromStartEnd(TIME_0830AM, TIME_0845AM, false),
Arrays.asList(PERSON_B)));

MeetingRequest request = new MeetingRequest(Arrays.asList(PERSON_A), DURATION_30_MINUTES);
request.addOptionalAttendee(PERSON_B);

Collection<TimeRange> actual = query.query(events, request);
Collection<TimeRange> expected =
Arrays.asList(TimeRange.fromStartDuration(TIME_0830AM, DURATION_30_MINUTES));

Assert.assertEquals(expected, actual);
}

public void onlyOptionalWithTime() {
// Two optional attendees with gaps in their schedules.
//
// Events : |--A--| |--B--|
// Day : |-----------------------------|
// Options : |--1--| |--2--| |--3--|

Collection<Event> events = Arrays.asList(
new Event("Event 1", TimeRange.fromStartEnd(TIME_0800AM, TIME_0900AM, false),
Arrays.asList(PERSON_A)),
new Event("Event 2", TimeRange.fromStartEnd(TIME_1000AM, TIME_1100AM, false),
Arrays.asList(PERSON_B)));

MeetingRequest request = new MeetingRequest(Arrays.asList(), DURATION_30_MINUTES);
request.addOptionalAttendee(PERSON_A);
request.addOptionalAttendee(PERSON_B);

Collection<TimeRange> actual = query.query(events, request);
Collection<TimeRange> expected =
Arrays.asList(TimeRange.fromStartEnd(TimeRange.START_OF_DAY, TIME_0800AM, false),
TimeRange.fromStartEnd(TIME_0900AM, TIME_1000AM, false),
TimeRange.fromStartEnd(TIME_1100AM, TimeRange.END_OF_DAY, true));

Assert.assertEquals(expected, actual);
}

public void onlyOptionalWithNoTime() {
// Two optional attendees with no gaps in their schedules. Should return empty.
//
// Events : |-----A-----||-------B--------|
// Day : |-----------------------------|
// Options :

Collection<Event> events = Arrays.asList(
new Event("Event 1", TimeRange.fromStartEnd(TimeRange.START_OF_DAY, TIME_1000AM, false),
Arrays.asList(PERSON_A)),
new Event("Event 2", TimeRange.fromStartEnd(TIME_1000AM, TimeRange.END_OF_DAY, true),
Arrays.asList(PERSON_B)));

MeetingRequest request = new MeetingRequest(Arrays.asList(), DURATION_30_MINUTES);
request.addOptionalAttendee(PERSON_A);
request.addOptionalAttendee(PERSON_B);

Collection<TimeRange> actual = query.query(events, request);
Collection<TimeRange> expected = Arrays.asList();

Assert.assertEquals(expected, actual);
}
}