views:

284

answers:

4

I'm trying to model a basic scenario involving a Person and a Seat. A Person has a Status property: Sitting or Standing. A seat has a Seated property that specifies the Person that is currently sitting in it. Also, a Seat is special in that it only "accepts" certain people to sit in it. I know it sounds strange for a Seat to "accept" someone, but just imagine it prefers certain people over others.

Following "Tell, Don't Ask," How should I design the Person and Seat objects so that a Person can sit down in a Seat only when the Seat "accepts" him and also have his status changed to Sitting. My first thought was that a Person should have a SitDown method as follows:

Person.SitDown(Seat seat);

But this seems like it would require the Person class to inspect the state of the Seat before sitting in it, as well as having to update the Seat's Seated property (instead of the Seat updating the property itself):

// inside the Person class
void SitDown(Seat seat) {
    if (seat.AcceptsPlayer(this)) {
        seat.Seated = this;
        this.Status = Sitting;
    }
}

It seems better to have the Seat class handle seating a person:

Seat.SeatPerson(Person person);

// inside Seat class
void SeatPerson(Person person) {
    if (IsAccepted(person)) {
        this.Seated = person;
        person.Status = Sitting;
    }
}

But this still requires the Seat to change the person's status. Is this the way that the person's status should be updated? Should only a Person be able to change his status? How would you model this simple scenario?

+3  A: 

Introduce a 3rd model... Seatings that has a reference to both the seat, and the person. Then you can create an instance of that model every time someone sits down, throw in some validations for preventing two people sitting in the same seat, and maybe even throw in some timeouts (if your sitting in a seat too long, you lose it).

diclophis
Can you provide a code example? I am having difficulty understanding how a Person's Status changes from Standing to Sitting using the Seatings model as you have specified.
snazzer
Instead of having a Status flag as a member variable, the Person would have an IsSeated() method that checks the Seatings model. Likewise the Seat would have an IsOccupied() method.
David Moles
A: 

let the person try to seat on the seat, and update it's state depending on the success of the operation:

Just call myPerson.TrySeat(targetseat), which returns true if the sitting process succeeded.

//inside Person class
        public bool TrySeat(Seat seat)
        {
            if (seat.TrySeat(this))
            {
                Status = Sitting;
                return true;
            }
            else
            {
                return false;
            }
        }

//inside Seat class
        internal bool TrySeat(Person person)
        {
            if (CanSeat(person))
            {
                Seated = person;
                return true;
            }
            else
            {
                return false;
            }
        }
Brann
I think that's precisely what he's trying to avoid. You also introduced a clone, i.e. a method which is very similar to another method elsewhere.
Frans Bouma
A: 

The problem is your model is defined with a circular dependency. There are two ways of avoiding this.

The first one doesn't exactly follow "Tell, Don't Ask" explicitly, but it gets closer to the point. We try to find out if we can sit down, then tell the chair that we're sitting in it.

void Person.SitDown(Seat seat) {
    if (seat.AcceptsPlayer(this)) {
        seat.SeatPerson(this);
        this.Status = Status.Sitting;
    }
}

void Seat.SeatPerson(Person person) {
    this.Seated = person;
}

A better way to do this (which follows the "Tell, Don't Ask" more explicitly) might be the following. We try to sit in the chair. If the chair rejects us, we know.

void Person.SitDown(Seat seat) {
    if (seat.SeatPerson(this)) {
        this.Status = Status.Sitting;
    }
    else
    {
        //Couldn't sit down!
    }
}

bool Seat.SeatPerson(Person person) {
    if (this.IsAccepted(person) && this.Seated == null) {
        this.Seated = person;
        return true;
    }
    else
    {
        return false;
    }
}
lc
The first example looks like it has a race condition. AcceptsPlayer() and SeatPerson() should, together, be an atomic operation -- cf. your second example with `IsAccepted()` private to `Seat`, instead of a public `AcceptsPlayer()`.
David Moles
A: 

Use a callback so that each class can maintain the state that it is responsible for.

public class Seat
{
  public void SeatPerson(Person person, Action successAction)
  {
    if (IsAccepted(person))
    {
      this.Seated = person;
      successAction();
    }
  }
}


public class Person
{
  public void Sit(Seat seat)
  {
    seat.SeatPerson(this, this.SitComplete);
  }

  public void SitComplete()
  {
    this.Status = Sitting;
  }
}

There's still cyclical dependency here.

Seat has a responsibility to check that the person attempting to sit is valid to do so. Seat carries a reference to the person once they have sat. Person only knows a method to try to sit in a seat.

By convention, successAction should not be held on to longer than the SeatPerson call. This guarantees that Seat cannot compromise the state of Person.

David B