1

I have a MeetingRoom object that contains a list of Employees, there is a function on each employee called SetMeetingDate, this method has to take into account the state of all other employees in the MeetingRoom in particicular the IsReadyForMeeting property.

An individual employee for example can only successfully call SetMeetingDate(Date date) when every employee in the ChatRoom has IsReadyForMeeting set to true.

I have thought about a couple of ways to implement this functionality, I can do something where the employee checks the state of the other employees by going via the MeetingRoom object like this:

    public void SetMeetingDate(DateTime date)
    {
        if (!MeetingRoom.AllEmployeesAreReady)
        {
            throw new ApplicationException("all employees are not ready");
        }

        MeetingDate = date;
    }

which seems to me to have the responsibility in the wrong place.

A second approach that I can use is to set up an observer so that the Employee class will inform the MeetingRoom object that it has set a meeting date and the MeetingRoom class will raise the exception if MeetingRoom.AllEmployeesAreReady is false thus moving the responsibility to the MeetingRoom object and the code for the employee would look like this:

    public void SetMeetingDate(DateTime date)
    {
        MeetingDate = date;

        Observer.MeetingDateAdded();
    }

And the Meeting Room would then have a method like this:

    public void MeetingDateAdded()
    {
        if (!AllEmployeesAreReady)
        {
            throw new ApplicationException("all employees are not ready");
        }
    }

This use of the observer has been met with opposition from other developers in my team and I can see why but I can't think of a better way to implement the requirement, is there an easy/different approach that can be used in this situation?

  • You're exception message would be improved if you'd make it something along "not all employees are ready" since your condition is that ALL employees must be ready, not some. – Niklas Rosenstein Feb 3 '15 at 18:30
  • This was a completely contrived example in reality I wouldn't use an ApplicationException but something more meaningful – nickbw Feb 3 '15 at 22:21
  • The entire first paragraph sounds wrong. Why does a meeting room reference employees (except possibly those in there right now)? That's the meeting's job. Why do employees have a SetMeetingDate? That's also the meeting's job. A meeting should be a combination of attendees, location, and time, and it's the meeting's job to make sure these all check out, like all attendees actually being available at the given time, the location being available, etc. – Sebastian Redl Feb 4 '15 at 12:48
  • You are right each Meeting would have a single date, but each employee can propose a date for themselves (SetMeetingDate) but they can only propose it if the rest of the Employees are in the correct state; this was an example that I came up with to illustrate the problem we were facing, it isn't meant to be real-world – nickbw Feb 4 '15 at 22:22
3

This doesn't look like a reasonable place to apply the Observer Pattern.

What I do notice is that all of your examples reference local state, so I assume they are part of some sort of Meeting object. Consequently, the code

public void SetMeetingDate(DateTime date)
{
    if (!MeetingRoom.AllEmployeesAreReady)
    {
        throw new ApplicationException("all employees are not ready");
    }

    MeetingDate = date;
}

is perfectly valid, since MeetingRoom and MeetingDate are clearly local properties of your "Meeting" class.

If you prefer something a bit more self-contained, then do something like this:

public static MeetingDate SetMeetingDate(MeetingRoom room, DateTime date)
{
    if (!room.AllEmployeesAreReady)
        throw new InvalidOperationException("All employees are not ready");

    return new MeetingDate(date, room.Employees);
}

or somesuch.

3

You should go with your first solution.

Your first solution is better because it's simpler (but maybe show the user a message instead of throwing an exception if it seems more appropriate).

Head First Design Patterns says it best:

First of all, when you design, solve things in the simplest way possible. Your goal should be simplicity, not "how can I apply a pattern to this problem." Don't feel like you aren't a sophisticated developer if you don't use a pattern to solve a problem. Other developers will appreciate and admire the simplicity of your design. That said, sometimes the best way to keep your design simple and flexible is to use a pattern.

It's usually best to use design patterns only when your alternatives all seem far too complicated. Let KISS and YAGNI be your guides when it comes to deciding whether to use a design pattern.

  • +1 Your goal should be simplicity, not "how can I apply a pattern to this problem." <-- this. It ought to be the opening line of every CS course on patterns. – Julia Hayward Feb 4 '15 at 9:07
0

I'd take a different approach, hopefully a clearer one.

// This is a filter function, needlessly wordy in Java.
List<Employee> getUnreadyParticipants(DateTime meetingMoment) {
  ArrayList<Employee> unreadyParticipants = new ArrayList<>();
  for (Employee e : this.getMeetingParticipants()) {
    if (!e.readyForMeeting(meetingTime)) unreadyParticipants.add(e);
  }
  return unreadyParticipants;
}

public void setMeetingDate(DateTime meetingMoment) {
  ArrayList<Employee> unreadyParticipants = getUnreadyParticipants(meetingMoment);
  if (unreadyParticipants.size() > 0) {
    throw new InvalidOperationException(
      "Participants not ready: " + printListNicely(unreadyParticipants)
    );
  }
  // whatever else is the logic
}

Reasons:

  • Checking time compatibility is MeetingRoom's concern, not Employee's.
  • The list of unready participants is available for diagnostics.
  • There are no flags and other pieces of state that denote readiness (like .AreAllEmployeesReady) which may go out of sync with the list of participants.
  • 1
    Essentially AreAllEmployeesReady would be a linq filter, and would have been implemented something like this return !Employees.Any(e => e.IsReadyToBook) so the only way that could get out of sync would be if the list of employees on the meeting room object got out of sync which would be a whole other issue – nickbw Feb 3 '15 at 22:45

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service, privacy policy and cookie policy

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