Skip to content

Optimized scheduling service to maximize optional attendees #32

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

Optimized scheduling service to maximize optional attendees #32

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
17 commits
Select commit Hold shift + click to select a range
702eef1
added tests
apluscs Jun 16, 2020
462c5ef
doesn't work
apluscs Jun 17, 2020
faac3cc
passes all tests but unreadable
apluscs Jun 17, 2020
137830a
documentation part 1
apluscs Jun 17, 2020
07709ab
extracted out functions from getOptimalMeetingTimes
apluscs Jun 17, 2020
0173049
docs mostly done, just add a few more @param's
apluscs Jun 17, 2020
4ca8972
docs done
apluscs Jun 17, 2020
a46fdd8
refactored code to shorten it, passes all tests
apluscs Jun 18, 2020
ed72b38
solution 2 fails double booking test... must revise
apluscs Jun 18, 2020
951e93b
solution 3 passes all tests
apluscs Jun 18, 2020
3b4e919
more readable
apluscs Jun 18, 2020
b9eccf9
tweaks
apluscs Jun 19, 2020
88b11de
encapsulated logic into SingleMeetingResolver
apluscs Jun 19, 2020
7a8941b
tweaks 2
apluscs Jun 19, 2020
cce1015
docu tweak
apluscs Jun 19, 2020
857fd09
tweaks
Jun 26, 2020
bd55190
cleaned up for loop
Jun 26, 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
288 changes: 198 additions & 90 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 @@ -18,15 +18,21 @@
import java.util.Arrays;
import java.util.Collection;
import java.util.Collections;
import java.util.stream.Collectors;
import java.util.Comparator;
import java.util.HashSet;
import java.util.HashMap;
import java.util.Iterator;
import java.util.List;
import java.util.Map;
import java.util.Set;
import java.util.TreeMap;

/** Lists possible meeting times based on meeting information it takes in. */
/**
* Supports a query function that lists optimal meeting times based on meeting information it takes
* in.
*/
public final class FindMeetingQuery {
private static final int END_OF_DAY = TimeRange.getTimeInMinutes(23, 59);

private static final Comparator<Event> ORDER_BY_START_ASC =
new Comparator<Event>() {
@Override
Expand All @@ -35,106 +41,208 @@ public int compare(Event a, Event b) {
}
};

/**
* Returns a list of time periods in which the meeting, specified by request, could happen. If one
* or more time slots exists so that both mandatory and optional attendees can attend, it returns
* those time slots. Otherwise, it returns the time slots that fit just the mandatory attendees.
*
* @param eventsCollection the events we know about
* @param request information about the meeting, including attendees, optional attendees, and how
* long it needs to be
*/
public Collection<TimeRange> query(Collection<Event> eventsCollection, MeetingRequest request) {
Collection<TimeRange> withOptionalAttendees = getMeetingTimes(eventsCollection, request, true);

// Special case: if no mandatory attendees and optional attendees' schedules cannot fit in a
// meeting, no meeting times are possible.
return !withOptionalAttendees.isEmpty() || request.getAttendees().isEmpty()
? withOptionalAttendees
: getMeetingTimes(eventsCollection, request, false);
}
// All logic needed to find optimal meeting times with one instance of meeting information and
// events to be considered.
private static class SingleMeetingResolver {
// events are all the events to be considered.
private final Collection<Event> events;

private Collection<TimeRange> getMeetingTimes(
Collection<Event> eventsCollection,
MeetingRequest request,
boolean includeOptionalAttendees) {
HashSet<String> attendees = new HashSet<String>(request.getAttendees());
if (includeOptionalAttendees) {
attendees.addAll(request.getOptionalAttendees());
/* request contains information about the meeting, including attendees, optional attendees, and how long it needs to be.
*/ private final MeetingRequest request;
private ArrayList<TimeRange> optimalMeetingTimes = new ArrayList<TimeRange>();
private long minMeetingDuration;

SingleMeetingResolver(Collection<Event> events, MeetingRequest request) {
this.events = events;
this.request = request;
this.minMeetingDuration = request.getDuration();
}
ArrayList<Event> events = getRelevantEvents(attendees, new ArrayList<Event>(eventsCollection));
List<TimeRange> possibleMeetingTimes = new ArrayList<TimeRange>();

// Need to check this so we don't access out of bounds when we add first gap.
if (events.isEmpty()) {
addIfLongEnough(
TimeRange.fromStartEnd(0, END_OF_DAY, true), possibleMeetingTimes, request.getDuration());
return possibleMeetingTimes;
/**
* Returns a list of time periods in which the meeting, specified by request, could happen. If
* no time exists for all optional and mandatory attendees, find the time slot(s) that allow
* mandatory attendees and the greatest possible number of optional attendees to attend.
*/
Collection<TimeRange> resolveBestTime() {
ArrayList<TimeRange> mandatoryAttendeesMeetingTimes =
getMandatoryAttendeesMeetingTimes(new HashSet<String>(request.getAttendees()));
getOptimalMeetingTimes(
mandatoryAttendeesMeetingTimes,
getChangesInOptionalAttendeesAttendance(
getOptionalAttendeesFreeTimes(new HashSet<String>(request.getOptionalAttendees()))));

// If there are no meeting times with at least one optional attendee, just return
// mandatoryAttendeesMeetingTimes.
return optimalMeetingTimes.isEmpty() ? mandatoryAttendeesMeetingTimes : optimalMeetingTimes;
}
Collections.sort(events, ORDER_BY_START_ASC);

// Add first gap.
addIfLongEnough(
TimeRange.fromStartEnd(0, events.get(0).getWhen().start(), false),
possibleMeetingTimes,
request.getDuration());
int end = events.get(0).getWhen().end();
for (Event event : events) {
// event can be merged with current time range
if (event.getWhen().start() <= end) {
end = Math.max(end, event.getWhen().end());
continue;

/**
* Returns all meeting times that allow the most optional attendees to attend. These times must
* also fall in a time satisfying all mandatory attendees and must be at least
* minMeetingDuration long. Utilizes two pointers: one to mandatoryAttendeesMeetingTimes, one to
* changeLog.
*
* @param mandatoryAttendeesMeetingTimes all meeting times satisfying mandatory attendees
* @param changes a change log of the number of available optional attendees over time
*/
private void getOptimalMeetingTimes(
ArrayList<TimeRange> mandatoryAttendeesMeetingTimes, TreeMap<Integer, Integer> changes) {
int mandatoryAttendeesMeetingTimesIndex = 0,
prevTime = 0,
bestAttendance = 0,
currAttendance = 0;
for (Map.Entry changeEntry : changes.entrySet()) {
// First need to back up mandatoryAttendeesMeetingTimesIndex in case we missed a time range
// in mandatoryAttendeesMeetingTimes.
mandatoryAttendeesMeetingTimesIndex = Math.max(0, mandatoryAttendeesMeetingTimesIndex - 1);
Copy link
Collaborator

@acmcarther acmcarther Jun 19, 2020 •
edited
Loading

Choose a reason for hiding this comment

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

This is not the clearest way to do this.

From what I can tell, you're intending to iterate over mandatoryAttendeesMeetingTimes only once, and you iterate over it over the course of iterating through changes. This "backtracking" is necessary because of the way you're iterating on the index variable.

The pattern I'd use for this is to (1) retain an iterator for mandatoryAttendeesMeetingTimes and (2) retain the "next" meeting time to be processed. Upon successful processing, get the next meeting time from the iterator, until there are no more remaining.

A skeleton of this:

Iterator<TimeRange> mandatoryMeetingTimesIter = mandatoryAttendeesMeetingTimes.iterator();
Optional<TimeRange> nextMandatoryMeetingTimeOpt = 
    mandatoryMeetingTimesIter.hasNext() ? Optional.of(mandatoryMeetingTimesIter.next()) : Optional.empty();
for (Map.Entry<String,String> changeEntry : changes.entrySet()) {
  while (nextMandatoryMeetingTimeOpt.isPresent()) {
    TimeRange thisMandatoryMeetingTime = nextMandatoryMeetingTImeOpt.get();
    // TODO: Explain what this check is doing
    if (thisMandatoryMeetingTime.start() <  changeEntry.getKey()) {
      break;
    }
    updateOptimalTimes(
        TimeRange.fromStartEnd(
            Math.max(thisMandatoryMeetingTime.start(),  prevTime),
            Math.min(thisMandatoryMeetingTime.end(),
                     (Integer) changeEntry.getKey()),
                     false));
    // Queue up the next meeting time.
    nextMandatoryMeetingTimeOpt = mandatoryMeetingTimesIter.hasNext() ?
                                                          Optional.of(mandatoryMeetingTimesIter.next()) : Optional.empty();
  }
  prevTime = (Integer) changeEntry.getKey();
  currAttendance += (Integer) changeEntry.getValue();
}
// TODO: What does it mean if nextMandatoryMeetingTimeOpt.isPresent() (still)? Handle this

Copy link
Owner Author

@apluscs apluscs Jun 19, 2020 •
edited
Loading

Choose a reason for hiding this comment

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

  1. l changed thisMandatoryMeetingTime.start() < changeEntry.getKey() to thisMandatoryMeetingTime.start() >= changeEntry.getKey() for logic reasons.
  2. l don't see how mandatoryMeetingTimesIter is going back by 1 at the start of the outer for loop. It also fails some tests, because of this reason (easiest to see is in the onlyOptionalAndDisjoint test).

l could accomplish this decrement by keeping a prevMandatoryMeetingTime and setting thisMandatoryMeetingTime to that at the top of every outer for loop but that seems clumsy. Or l could do something with a ListIterator, since it supports a previous() method. Any thoughts?

Copy link
Collaborator

@acmcarther acmcarther Jun 22, 2020 •
edited
Loading

Choose a reason for hiding this comment

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

l changed thisMandatoryMeetingTime.start() < changeEntry.getKey() to thisMandatoryMeetingTime.start() >= changeEntry.getKey() for logic reasons.

Acknowledged, though I don't see this check in the new code, so I'm really not sure what's going on.

l don't see how mandatoryMeetingTimesIter is going back by 1 at the start of the outer for loop. It also fails some tests, because of this reason (easiest to see is in the onlyOptionalAndDisjoint test).

This could be interpreted as "why doesn't your code do this" or "my code doesn't actually do this". I'll try to answer both, but I just wanted to give you this caveat that i didn't really actually understand your comment.

The solution I described doesn't require mandatoryMeetingTimesIter to "go back one", because it is not being incremented unless the value is consumed. This is the purpose behind storing "the next value", and incrementing that immediately after the call to updateOptimalTimes. This more closely matches the semantics you want, rather than the for loop construction, which always increments the value when the value (that it is an index to) doesn't always get consumed.

A do while might be equivalent and still use the index based construction (though please, if you do that, don't let it grow to be like five lines in the declaration like this one).

Copy link
Owner Author

@apluscs apluscs Jun 26, 2020 •
edited
Loading

Choose a reason for hiding this comment

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

Sorry for the confusion.

  1. The change in my first point was done locally and l didn't push that because it was failing tests.
  2. The crux is that l need to advance mandatoryMeetingTimesIter enough for it to be too far in the future but then still be able to go back to last one that was valid (that time could still be valid for the next one in the changeLog). l think l get what you're saying now, with looking ahead to see the next mandatoryMeetingTime and choosing to advance or not. But in your code, when you "Queue up the next meeting time", you advance the iterator and you're unable to go back = bugs. l can use your iterator method but l'll have to make some changes from the code you proposed.

Copy link
Owner Author

@apluscs apluscs Jun 26, 2020

Choose a reason for hiding this comment

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

Nvm, iterators are annoyingly verbose. l'll take you up on the while loop idea.


// Then compare time range from previous time in changeLog to current time in changeLog
// with mandatoryAttendeesMeetingTimes that overlap with this time range.
while (mandatoryAttendeesMeetingTimesIndex < mandatoryAttendeesMeetingTimes.size()
&& mandatoryAttendeesMeetingTimes.get(mandatoryAttendeesMeetingTimesIndex).start()
< (int) changeEntry.getKey()) {
TimeRange meetingRange =
TimeRange.fromStartEnd(
Math.max(
mandatoryAttendeesMeetingTimes
.get(mandatoryAttendeesMeetingTimesIndex)
.start(),
prevTime),
Math.min(
mandatoryAttendeesMeetingTimes.get(mandatoryAttendeesMeetingTimesIndex).end(),
(int) changeEntry.getKey()),
false);
mandatoryAttendeesMeetingTimesIndex++;
if (meetingRange.duration() < minMeetingDuration) {
continue;
}

// Clear out all former optimal meeting times. They aren't the most optimal anymore.
if (currAttendance > bestAttendance) {
bestAttendance = currAttendance;
optimalMeetingTimes.clear();
}
if (currAttendance == bestAttendance) {
optimalMeetingTimes.add(meetingRange);
}
}
prevTime = (int) changeEntry.getKey();
currAttendance += (int) changeEntry.getValue();
}
// Add the time range we were tracking, start a new one from event.
addIfLongEnough(
TimeRange.fromStartEnd(end, event.getWhen().start(), false),
possibleMeetingTimes,
request.getDuration());
end = event.getWhen().end();
}

// Add the last one we were tracking.
addIfLongEnough(
TimeRange.fromStartEnd(end, END_OF_DAY, true), possibleMeetingTimes, request.getDuration());
return possibleMeetingTimes;
}
/**
* Returns a mapping of optional attendees to their free times.
*
* @param optionalAttendees everyone who needs to attend this meeting
*/
private HashMap<String, ArrayList<TimeRange>> getOptionalAttendeesFreeTimes(
HashSet<String> optionalAttendees) {
HashMap<String, ArrayList<TimeRange>> times = new HashMap<String, ArrayList<TimeRange>>();
for (String attendee : optionalAttendees) {
HashSet<String> attendeeSet = new HashSet<String>();
attendeeSet.add(attendee);

/**
* Adds range to ranges if it is long enough to fit in a meeting.
*
* @param range the range being considered
* @param ranges the list of ranges >= meetingDuration
* @param meetingDuration the duration of meeting to be scheduled
*/
private static void addIfLongEnough(
TimeRange range, List<TimeRange> ranges, long meetingDuration) {
if (range.duration() >= meetingDuration) {
ranges.add(range);
// Find all possible meeting times for just this one attendee. Must do this to deal with
// double bookings.
times.put(attendee, getMandatoryAttendeesMeetingTimes(attendeeSet));
}
return times;
}
}

/**
* Returns only those events that are attended by at least one attendee that is attending the
* meeting we are trying to schedule. More intuitively, an event is "relevant" if it is attended
* by at least one "relevant" attendee.
*
* @param requestAttendees the set of attendees attending the meeting ("relevant" people)
*/
private static ArrayList<Event> getRelevantEvents(
HashSet<String> relevantAttendees, ArrayList<Event> events) {
ArrayList<Event> relevantEvents = new ArrayList<Event>();
for (Event event : events) {
boolean isRelevant = false;
for (String person : event.getAttendees()) {
if (relevantAttendees.contains(person)) {
isRelevant = true;
break;
/**
* Returns a change log of the number of available optional attendees over time. First part of a
* sweep-line algorithm.
*
* @param optionalAttendeesFreeTimes mapping of all attendees to their free times
*/
private TreeMap<Integer, Integer> getChangesInOptionalAttendeesAttendance(
HashMap<String, ArrayList<TimeRange>> optionalAttendeesFreeTimes) {
TreeMap<Integer, Integer> changes = new TreeMap<Integer, Integer>();
for (Map.Entry e : optionalAttendeesFreeTimes.entrySet()) {
for (TimeRange time : (ArrayList<TimeRange>) e.getValue()) {
changes.put(time.start(), changes.getOrDefault(time.start(), 0) + 1);
changes.put(time.end(), changes.getOrDefault(time.end(), 0) - 1);
}
}
if (isRelevant) {
relevantEvents.add(event);
return changes;
}

/**
* Returns all meeting times that satisfy all mandatory attendees of request. Sorted in
* ascending order.
*
* @param mandatoryAttendees everyone who needs to attend this meeting
*/
private ArrayList<TimeRange> getMandatoryAttendeesMeetingTimes(
HashSet<String> mandatoryAttendees) {
ArrayList<Event> relevantEvents = getRelevantEvents(mandatoryAttendees);
ArrayList<TimeRange> possibleMeetingTimes = new ArrayList<TimeRange>();

// Need to check this so we don't access out of bounds when we add first gap.
if (relevantEvents.isEmpty()) {
addIfLongEnough(
TimeRange.fromStartEnd(0, TimeRange.END_OF_DAY, true), possibleMeetingTimes);
return possibleMeetingTimes;
}
Collections.sort(relevantEvents, ORDER_BY_START_ASC);

// Add first gap.
addIfLongEnough(
TimeRange.fromStartEnd(0, relevantEvents.get(0).getWhen().start(), false),
possibleMeetingTimes);
int end = relevantEvents.get(0).getWhen().end();
for (Event event : relevantEvents) {
// event can be merged with current time range
if (event.getWhen().start() <= end) {
end = Math.max(end, event.getWhen().end());
continue;
}
// Add the time range we were tracking, start a new one from event.
addIfLongEnough(
TimeRange.fromStartEnd(end, event.getWhen().start(), false), possibleMeetingTimes);
end = event.getWhen().end();
}

// Add the last one we were tracking.
addIfLongEnough(
TimeRange.fromStartEnd(end, TimeRange.END_OF_DAY, true), possibleMeetingTimes);
return possibleMeetingTimes;
}
return relevantEvents;

/**
* Adds range to ranges if it is long enough to fit in a meeting.
*
* @param range the range being considered
* @param ranges the list of ranges >= meetingDuration
*/
private void addIfLongEnough(TimeRange range, List<TimeRange> ranges) {
if (range.duration() >= minMeetingDuration) {
ranges.add(range);
}
}

/**
* Returns only those events that are attended by at least one attendee that is attending the
* meeting we are trying to schedule. More intuitively, an event is "relevant" if it is attended
* by at least one "relevant" attendee.
*
* @param relevantAttendees the set of attendees attending the meeting ("relevant" people)
*/
private ArrayList<Event> getRelevantEvents(HashSet<String> relevantAttendees) {
return new ArrayList<Event>(
events.stream()
.filter(
e ->
e.getAttendees().stream()
.filter(relevantAttendees::contains)
.findAny()
.isPresent())
.collect(Collectors.toList()));
}
}

public Collection<TimeRange> query(Collection<Event> events, MeetingRequest request) {
return new SingleMeetingResolver(events, request).resolveBestTime();
}
}
Loading