Optimized scheduling service to maximize optional attendees

[apluscs]

No description provided.

[jalexanderqed]

This review is not 100% complete, but I may not be able to get back to this for a while, so sending now.

[acmcarther]

I'd like to review again once the variables I whined about have better names.

[acmcarther]

You've resolved a bunch of comments but not actually pushed the changes.

[acmcarther]

Nevermind, I got myself tricked by clicking an outdated comment...

[acmcarther]

going to need another pass.

[acmcarther]

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

[apluscs]

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?

[acmcarther]

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

[apluscs]

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.

[apluscs]

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

[acmcarther]

You might need jqed to review this to approval. I can't understand what is going on here.