Skip to content

Added tests for optional attendees and modified query to handle optional attendees #22

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
GrantComm wants to merge 4 commits into
base: master
Choose a base branch
from
Open

Added tests for optional attendees and modified query to handle optional attendees #22

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
9816056
Working on tests
GrantComm Jun 18, 2020
ac25324
Fails 2 tests
GrantComm Jun 19, 2020
0fc7df6
Passes all tests
GrantComm Jun 19, 2020
cb5d560
Minor fixes
GrantComm Jun 19, 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
42 changes: 30 additions & 12 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 @@ -28,28 +28,45 @@ public final class FindMeetingQuery {
public int compare(Event a, Event b) {
return Long.compare(a.getWhen().start(), b.getWhen().start());
}};

public Collection<TimeRange> query(Collection<Event> eventCollection, MeetingRequest request) {

public Collection<TimeRange> query(Collection<Event> eventCollection, MeetingRequest request) {
Collection<String> attendees = new ArrayList<String>();
Copy link
Collaborator

@acmcarther acmcarther Jun 19, 2020

Choose a reason for hiding this comment

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

Ah, heres the collection.

Yeah, I said before to use generic types, but Collection is too generic, lol. List is probably what you want.

attendees.addAll(request.getOptionalAttendees());
attendees.addAll(request.getAttendees());
if (request.getOptionalAttendees().isEmpty()) {
attendees.removeAll(request.getOptionalAttendees());
}
Comment on lines +36 to +38
Copy link
Collaborator

@jalexanderqed jalexanderqed Jun 19, 2020

Choose a reason for hiding this comment

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

This seems unnecessary. You would be removing nothing?

if (request.getAttendees().isEmpty()) {
attendees.removeAll(request.getAttendees());
}
Comment on lines +39 to +41
Copy link
Collaborator

@jalexanderqed jalexanderqed Jun 19, 2020

Choose a reason for hiding this comment

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

The same here.

if (queryHelper(eventCollection, request, attendees).isEmpty()) {
attendees.removeAll(request.getOptionalAttendees());
return queryHelper(eventCollection, request, attendees);
}
return queryHelper(eventCollection, request, attendees);
Comment on lines +42 to +46
Copy link
Collaborator

@jalexanderqed jalexanderqed Jun 19, 2020

Choose a reason for hiding this comment

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

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.

}

private static Collection<TimeRange> queryHelper(Collection<Event> groupOfEvents, MeetingRequest request, Collection<String> attendees) {
Copy link
Collaborator

@jalexanderqed jalexanderqed Jun 19, 2020

Choose a reason for hiding this comment

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

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.

Copy link
Collaborator

@acmcarther acmcarther Jun 19, 2020

Choose a reason for hiding this comment

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

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<>.

// Too long of a request
if (request.getDuration() > TimeRange.WHOLE_DAY.duration()) {
return Collections.emptyList();
}

// No events or attendees
List<Event> events = new ArrayList<Event>(eventCollection);
List<String> meetingAttendees = new ArrayList<String>(request.getAttendees());
if (events.isEmpty() || meetingAttendees.isEmpty()) {
// No events
List<Event> events = new ArrayList<Event>(groupOfEvents);
Copy link
Collaborator

@acmcarther acmcarther Jun 19, 2020

Choose a reason for hiding this comment

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

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.

if (events.isEmpty()) {
return Arrays.asList(TimeRange.WHOLE_DAY);
}


// Ignores unattended events
List<Event> importantEvents = new ArrayList<Event>();
for (Event event : events) {
if (!Collections.disjoint(event.getAttendees(), request.getAttendees())) {
if (!Collections.disjoint(event.getAttendees(), attendees)) {
importantEvents.add(event);
}
}

// If the list of important events is empty, return the whole day
if (importantEvents.isEmpty()) {
return Arrays.asList(TimeRange.WHOLE_DAY);
Expand All @@ -62,7 +79,7 @@ public Collection<TimeRange> query(Collection<Event> eventCollection, MeetingReq
List<TimeRange> acceptableMeetingTimes = new ArrayList<TimeRange>();

// Add the event that starts first
acceptableMeetingTimes.add(TimeRange.fromStartEnd(0, importantEvents.get(0).getWhen().start(), false));
acceptableMeetingTimes.add(TimeRange.fromStartEnd(TimeRange.START_OF_DAY, importantEvents.get(0).getWhen().start(), false));

// Set the end time and start time as the end of the first event
int latestEventEnd = importantEvents.get(0).getWhen().end();
Expand All @@ -72,15 +89,15 @@ public Collection<TimeRange> query(Collection<Event> eventCollection, MeetingReq
for (Event event : importantEvents) {
start = event.getWhen().start();

if (start > latestEventEnd) {
if (start >= latestEventEnd) {
if (rangeLessThanDuration(TimeRange.fromStartEnd(latestEventEnd, start, false), request.getDuration())) {
acceptableMeetingTimes.add(TimeRange.fromStartEnd(latestEventEnd, start, false));
latestEventEnd = event.getWhen().end();
}
latestEventEnd = event.getWhen().end();
}

if (start < latestEventEnd) {
if (start <= latestEventEnd) {
Copy link
Collaborator

@acmcarther acmcarther Jun 19, 2020

Choose a reason for hiding this comment

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

Nit: This = here isn't doing anything since you're detecting == above.

if (latestEventEnd < event.getWhen().end()) {
latestEventEnd = event.getWhen().end();
if (rangeLessThanDuration(TimeRange.fromStartEnd(latestEventEnd, start, false), request.getDuration())) {
Expand Down Expand Up @@ -110,11 +127,12 @@ public Collection<TimeRange> query(Collection<Event> eventCollection, MeetingReq
}
}

return acceptableMeetingTimes;
return acceptableMeetingTimes;
}

// Check if the meeting duration fits within a given time range
private static boolean rangeLessThanDuration(TimeRange range, long meetingDuration) {
return (range.duration() >= meetingDuration);
}

Copy link
Collaborator

@jalexanderqed jalexanderqed Jun 19, 2020

Choose a reason for hiding this comment

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

Rm extra line

}
124 changes: 119 additions & 5 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,15 +34,20 @@ 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);
private static final int TIME_1030AM = TimeRange.getTimeInMinutes(10, 30);
private static final int TIME_1100AM = TimeRange.getTimeInMinutes(11, 00);

private static final int TIME_1130AM = TimeRange.getTimeInMinutes(11, 30);

private static final int DURATION_15_MINUTES = 15;
private static final int DURATION_30_MINUTES = 30;
private static final int DURATION_60_MINUTES = 60;
private static final int DURATION_90_MINUTES = 90;
Expand Down Expand Up @@ -209,16 +214,16 @@ public void justEnoughRoom() {
// Options : |-----|

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

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

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

Assert.assertEquals(expected, actual);
}
Expand Down Expand Up @@ -270,5 +275,114 @@ public void notEnoughRoom() {

Assert.assertEquals(expected, actual);
}
}

@Test
public void optionalAttendeeConsidered() {
// The optional attendee has gaps that allow a meeting with the required attendees
Collection<Event> events = Arrays.asList(
new Event("Event 1", TimeRange.fromStartDuration(TIME_1000AM, DURATION_30_MINUTES),
Arrays.asList(PERSON_A)),
new Event("Event 2", TimeRange.fromStartDuration(TIME_1100AM, DURATION_30_MINUTES),
Arrays.asList(PERSON_B)),
new Event("Event 3", TimeRange.fromStartDuration(TIME_1030AM, 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_1000AM, false),
TimeRange.fromStartEnd(TIME_1130cAM, TimeRange.END_OF_DAY, true));

Assert.assertEquals(expected, actual);
}

@Test
public void optionalAttendeeNotConsidered() {
// Optional attendee has no time to meet and is therefore not considered
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.WHOLE_DAY, 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 noRoomForOptionalAttendee() {
// Optional Attendee has a meeting that results in the time slot available
// for attendees being too small
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, true),
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);
}

@Test
public void twoOptionalAttendeesWithGaps() {
// Only two optional attendees are considered who have gaps in their schedule
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)));

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_0830AM, TIME_0900AM, false),
TimeRange.fromStartEnd(TIME_0930AM, TimeRange.END_OF_DAY, true));

Assert.assertEquals(expected, actual);
}

@Test
public void twoOptionalAttendeesWithNoGaps() {
// Only two optional attendees and no gaps during the day
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_B)));

MeetingRequest request = new MeetingRequest(Arrays.asList(PERSON_A, PERSON_B), DURATION_60_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);
}

}
4 changes: 2 additions & 2 deletions walkthroughs/week-5-tdd/project/src/test/java/com/google/sps/MeetingRequestTest.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -37,8 +37,8 @@ public final class MeetingRequestTest {

@Test
public void CantAddOptionalAttendeeWhoIsAlsoMandatory() {
MeetingRequest request = new MeetingRequest(Arrays.asList(PERSON_A), DURATION_1_HOUR);
request.addOptionalAttendee(PERSON_A);
MeetingRequest request = new MeetingRequest(Arrays.asList(PERSON_C), DURATION_1_HOUR);
request.addOptionalAttendee(PERSON_C);

int actual = request.getOptionalAttendees().size();
int expected = 0;
Expand Down