Algorithm that tells the user what current "status" they fall under based on their current time in comparison to an array of meeting data times

This is an algorithm that takes in an ArrayList of objects that store information on meeting/event schedules. This algorithm's purpose is to check and see what the user's status is based on their current time.

The case scenarios are:

• The user's current time is before all meetings

• The user's current time is during a meeting (also handles meetings that start right when another meeting ends, also if the user is during the final meeting edge case).

• The user's current time is between two meetings

• The user's current time is after all meetings

• There is no meeting data.

After the algorithm determines which case scenario the user is in. It then assigns another object's "Availibility" times.

For instance, if a user's current time is found to be during an already existent meeting for that particular room. Then the "next available start time" for that room becomes the meeting's end time. And the 'next available end time" (i.e. when that room becomes busy again) is assigned to the beginning next meeting after the meeting that the current user's time is in the middle of. Handling of course edge cases like if two meetings immediately follow one another etc etc.

Here is the relevant code:

```
/**
* Method used by the MeetingDataTest test suite to check different time scenarios.
*/
public void testSetDefaultNextAvailable(ArrayList testMeetingData, DateTime caseToTest) {
this.meetingData = testMeetingData;
availableNow = false;
DateTime now = caseToTest;

//First check to see if there even are any meetings.
if (meetingData.size()!=0) {
//Check to see if there are meetings, if the user's current time/simulated time is before all meetings.
if (checkIfMorning(now)) {
System.out.println(nextAvailableStart.toString("H:mm a"));
if (nextAvailableEnd != null) {
System.out.println(nextAvailableEnd.toString("H:mm a"));
}
}
//If the user's current time is not found to

What you have done right:

• Seperate methods to check each case. Makes the main method easier to read.

• Make the dateTime to test against a parameter of the method. (easier to test)

What can be improved in testSetDefaultNextAvailable:

• This method, instead of returning a result, stores it internally in a field, that then needs to be queried. So, each calculation needs a new instance, or be sure that the same instance ins't reused on other threads.

• The now local variable is superfluous.

• Deeply nested if structure. And strangely the if else chains' bodies are all the same...

• Your code would really benefit from using a Range class of some sort. (I would recommend guava's range class).

• \$00\$ is \$0\$ but octal.

• Use of System.out.println(): return a result and let the client print it if needed. A calculation should just calculate.

• The boolean methods have side effects. Method names do not suggest it.

• 22 pm is hard coded as end of working day? Make this an input, or configuration.

• Hard coded time zone to be the default time zone. Make this an input, or configuration.

• meetingData ArrayList is used as internal state. Yet client code can still mess with the contents of this list, while the calculation is running.

• Method name can be improved.

What can be improved in checkIfAfterAllMeetings:

• This method assumes meetingData is properly sorted. Yet, this is not checked, nor is the list sorted by this class.

• What if now is 23 pm? nextAvailableEnd will be before nextAvailableStart.

• Method should be private (this goes for all but the main method)

What can be improved in checkIfMorning:

• A loop with a break to a label: not done. (occurs in other methods too)

• Logic is faulty, checks if there is at least one meeting after current time, not that all meetings are.

What can be improved in checkIfDuringMeeting:

• Enforce sorting of meeting data, this will make looking up 'the next meeting' simpler

What can be improved in checkIfBetweenMeetings:

• Imagine a Range abstraction that allows range.contains(dateTime).

Conclusion:

I think rewriting this as a method that returns a value (the current state of the calculation class), and using a Range abstraction (again referencing guava's Range and RangeSet) will make the logic a lot simpler. Which, in turn will make the naming easier.
As an added bonus the calculation will be stateless and automatically thread safe (provided you fix the meeting data encapsulation).

Edit in response to your questions:

• It doesn't really matter that this class is never touched by the user. By client I mean any code that calls the method. If that code is a test, then that test is the client.

• If the hard coded 22 pm is part of the test client's scenario, then this class doesn't need to define that again. I would still recommend this value to be an input or configuration.

• \$00\$ vs. \$0\$ is a very fine hair to split. It both amounts to an int with value \$0\$. Yet the \$00\$ literal is octal. (try \$09\$, it won't compile!) You were obviously trying to mimic the formatted time, and not aware of the difference. I remark it since you were likely not aware of this.

• ArrayList encapsulation: your method does this; this.meetingData = testMeetingData; i.e. the ArrayList is the same instance as passed to the method. If the client code modifies the ArrayList after this call (possibly even during the calculation) then the state of this class also changes, and post-conditions of the calculations may be corrupted.

I've also taken the time to code up how using guava's Range can simplify the logic a lot:

Range firstAvailableRange(List testMeetingData, DateTime now) {
    RangeSet freeTimes = toMeetingTimes(testMeetingData).complement();
    freeTimes.remove(Range.atMost(now));
    Iterator> iterator = freeTimes.asRanges().iterator();
    if (!iterator.hasNext()) {
        return null;
    }
    return iterator.next();
}

private TreeRangeSet toMeetingTimes(List testMeetingData) {
    TreeRangeSet meetingTimes = TreeRangeSet.create();
    for (MeetingData data : testMeetingData) {
        meetingTimes.add(data.getTimeRange());
    }
    return meetingTimes;
}

This doesn't cap to 22 pm yet, but I'm just trying to show how using a good Range abstraction can do most of the heavy lifting for you.

StackExchange Code Review Q#99053, answer score: 6