5
\$\begingroup\$

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:

  1. The user's current time is before all meetings
  2. 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).
  3. The user's current time is between two meetings
  4. The user's current time is after all meetings
  5. 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<MeetingData> 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 be before all meetings, next try to see if it is during any meetings.
        else if (checkIfDuringMeeting(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 be during any meetings either, next try to see if it is between any two meetings.
        else if (checkIfBetweenMeetings(now)) {
            System.out.println(nextAvailableStart.toString("H:mm a"));
            if (nextAvailableEnd != null) {
                System.out.println(nextAvailableEnd.toString("H:mm a"));
            }
        }
        //If the user is not between two meetings either, check to see if they are after all meetings.
        else if (checkIfAfterAllMeetings(now)) {
            System.out.println(nextAvailableStart.toString("H:mm a"));
            if (nextAvailableEnd != null) {
                System.out.println(nextAvailableEnd.toString("H:mm a"));
            }
        }
    }
    //No meeting data at all, the room is available all day!
    else {
        availableNow = true;
        nextAvailableEnd = new DateTime(now.getYear(), now.getMonthOfYear(), now.getDayOfMonth(), 22, 00, 00, DateTimeZone.getDefault());
        nextAvailableStart = now;
        System.out.println("Available all day");
    }

}


public boolean checkIfAfterAllMeetings(DateTime currentTime) {
    boolean isAfterAllMeetings = false;
    if (currentTime.isAfter(meetingData.get(meetingData.size() - 1).getEndTime())) {
        isAfterAllMeetings = true;
        availableNow=true;
        nextAvailableStart = currentTime;

        //Set to 10:00 PM if no match found
        nextAvailableEnd = new DateTime(currentTime.getYear(), currentTime.getMonthOfYear(), currentTime.getDayOfMonth(), 22, 00, 00, DateTimeZone.getDefault());
    }
    return isAfterAllMeetings;
}

/**
 * Checks to see if the user's current time or simulated time is between any two meetings.
 * Also sets the availableNow flag to true.
 *
 * @param currentTime
 * @return - True if the user's current time is between two meetings. False if not.
 */
public boolean checkIfBetweenMeetings(DateTime currentTime) {
    boolean isBetweenMeetings = false;
    loop:
    for (int i = 0; i < meetingData.size() - 1; i++) {
        if (currentTime.isBefore(meetingData.get(i + 1).getStartTime()) && currentTime.isAfter(meetingData.get(i).getEndTime())) {
            isBetweenMeetings = true;
            availableNow = true;
            nextAvailableStart = currentTime;
            nextAvailableEnd = meetingData.get(i + 1).getStartTime();
        }
    }
    return isBetweenMeetings;
}

/**
 * Checks to see if the user is before all meetings. If so, sets the time availability to now, enables the "availableNow" boolean flag
 * and then sets the end time availability for the beginning/start time of the first meeting.
 *
 * @param currentTime - the current time, simulated or actual.
 * @return - True if the user's current time/simulated time is before any meetings. False if not.
 */
public boolean checkIfMorning(DateTime currentTime) {
    boolean isBeforeAllMeetings = false;
    loop:
    for (int i = 0; i < meetingData.size() - 1; i++) {
        //case 1 - user is before any meetings.
        if (currentTime.isBefore(meetingData.get(0).getStartTime())) {
            nextAvailableStart = currentTime;
            nextAvailableEnd = meetingData.get(0).getStartTime();
            // Log.v("Case 1", nextAvailableEnd.toString("HH:mm a"));
            availableNow = true;
            isBeforeAllMeetings = true;
            break loop;
        }
    }
    return isBeforeAllMeetings;
}

/**
 * Checks to see if the user's current time is during a meeting. If it is it sets the corresponding next availabilities.
 * Also handles the caveat that the user is during the "final meeting of the day" and if that is the case, it sets the next available
 * start time to the final meeting's end time and the next available end time gets hard coded to 10:00 PM of the current day.
 * NOTE - This also handles meetings that are immediately followed by another meeting.
 *
 * @param currentTime - the current time, simulated or actual.
 * @return - True if the user's current time/simulated time is during any meetings. False if not.
 */
public boolean checkIfDuringMeeting(DateTime currentTime) {
    boolean isDuringMeeting = false;
    boolean isDuringFinalMeeting = false;
    MeetingData referenceMeeting = null;
    int indexOfFoundMeeting = 0;
    //SET THE REFERENCE POINT
    findMeetingThatTheCurrentTimeIsDuring:
    for (int i = 0; i < meetingData.size(); i++) {
        if (currentTime.isAfter(meetingData.get(i).getStartTime()) && currentTime.isBefore(meetingData.get(i).getEndTime())) {
            referenceMeeting = meetingData.get(i);
            indexOfFoundMeeting = i;
            isDuringMeeting = true;
            if (i == meetingData.size() - 1) {
                isDuringFinalMeeting = true;
            }
            break findMeetingThatTheCurrentTimeIsDuring;
        }
    }

    if (isDuringMeeting && !isDuringFinalMeeting) {
        loopFindNextAvailabilitiesUsingReferenceMeeting:
        for (int i = indexOfFoundMeeting + 1; i < meetingData.size() - 1; i++) {
            if (!referenceMeeting.getEndTime().isBefore(meetingData.get(i).getStartTime()) || referenceMeeting.getEndTime().isEqual(meetingData.get(i).getStartTime())) {
                referenceMeeting = meetingData.get(i);
                nextAvailableStart = referenceMeeting.getEndTime();
                nextAvailableEnd = meetingData.get(i + 1).getStartTime();
            }
        }
    } else if (isDuringFinalMeeting) {
        //Set to 10:00 PM if no match found
        nextAvailableEnd = new DateTime(currentTime.getYear(), currentTime.getMonthOfYear(), currentTime.getDayOfMonth(), 22, 00, 00, DateTimeZone.getDefault());
        nextAvailableStart = referenceMeeting.getEndTime();
    }
    return isDuringMeeting;
}

Useful background info:

  1. This is using JodaTime library for time comparisons.
  2. Incoming RoomData and MeetingData is parsed from JSON and constructed into an ArrayList.

What I'd like to improve on or receive feedback on:

  1. Better way to write algorithms if possible instead of nesting for-loops.
  2. Comments about code format
  3. Anything else really.
\$\endgroup\$
1
  • \$\begingroup\$ I believe that I have added all that should be "necessary" to understand the code. However if anyone needs more details or background please comment and I will answer ASAP. \$\endgroup\$ Commented Aug 4, 2015 at 20:15

1 Answer 1

6
\$\begingroup\$

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<DateTime> firstAvailableRange(List<MeetingData> testMeetingData, DateTime now) {
    RangeSet<DateTime> freeTimes = toMeetingTimes(testMeetingData).complement();
    freeTimes.remove(Range.atMost(now));
    Iterator<Range<DateTime>> iterator = freeTimes.asRanges().iterator();
    if (!iterator.hasNext()) {
        return null;
    }
    return iterator.next();
}

private TreeRangeSet<DateTime> toMeetingTimes(List<MeetingData> testMeetingData) {
    TreeRangeSet<DateTime> 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.

\$\endgroup\$
5
  • \$\begingroup\$ Thanks for the feedback, some of it is stuff that is obviously helpful but already done among other classes. Some feedback and details regarding what you wrote: Regarding most comments within testSetDefaultNextAvailable - This class is never touched by the user, it merely reflects a similar method. It is to test the functionality of hardcoded scenarios and test cases within our test suite. (Which explains the hardcoded timezones etc etc). - This app will be running internally and not be used by users after 10:00 PM so the 23rd hour should not make a different. \$\endgroup\$ Commented Aug 5, 2015 at 1:25
  • \$\begingroup\$ Some clarification I need: What do you mean by 00 is 0 but octal? why does that matter? (Sorry if I am missing something) What do you mean meetingData ArrayList is used as internal state yet client code can still mess with contents? This ArrayList is set before the application is even opened. And done with a callback after server retrieves the data. \$\endgroup\$ Commented Aug 5, 2015 at 1:26
  • \$\begingroup\$ On another note, do you think that my algorithms for matching room meeting data are sufficient? I couldn't really think of a more clever way than nesting loops which is disgusting... \$\endgroup\$ Commented Aug 5, 2015 at 1:30
  • \$\begingroup\$ Oddly enough JodaTime supports range abstractions. I should refactor some of this to incorporate that functionality. \$\endgroup\$ Commented Aug 5, 2015 at 1:31
  • \$\begingroup\$ also, the format of the output dates is hard-coded and will not work for half the planet. \$\endgroup\$
    – njzk2
    Commented Aug 5, 2015 at 3:31

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service and acknowledge you have read our privacy policy.

Not the answer you're looking for? Browse other questions tagged or ask your own question.