views:

3011

answers:

10

I want to ask a question about how you would approach a simple object-oriented design problem. I have a few ideas of my own about what the best way of tackling this scenario, but I would be interested in hearing some opinions from the Stack Overflow community. Links to relevant online articles are also appreciated. I'm using C#, but the question is not language specific.

Suppose I am writing a video store application whose database has a Person table, with PersonId, Name, DateOfBirth and Address fields. It also has a Staff table, which has a link to a PersonId, and a Customer table which also links to PersonId.

A simple object oriented approach would be to say that a Customer "is a" Person and therefore create classes a bit like this:

class Person {
    public int PersonId { get; set; }
    public string Name { get; set; }
    public DateTime DateOfBirth { get; set; }
    public string Address { get; set; }
}

class Customer : Person {
    public int CustomerId { get; set; }
    public DateTime JoinedDate { get; set; }
}

class Staff : Person {
    public int StaffId { get; set; }
    public string JobTitle { get; set; }
}

Now we can write a function say to send emails to all customers:

static void SendEmailToCustomers(IEnumerable<Person> everyone) { 
    foreach(Person p in everyone)
        if(p is Customer)
            SendEmail(p);
}

This system works fine until we have someone who is both a customer and a member of staff. Assuming that we don't really want our everyone list to have the same person in twice, once as a Customer and once as a Staff, do we make an arbitrary choice between:

class StaffCustomer : Customer { ...

and

class StaffCustomer : Staff { ...

Obviously only the first of these two would not break the SendEmailToCustomers function.

So what would you do?

  • Make the Person class have optional references to a StaffDetails and CustomerDetails class?
  • Create a new class that contained a Person, plus optional StaffDetails and CustomerDetails?
  • Make everything an interface (e.g. IPerson, IStaff, ICustomer) and create three classes that implemented the appropriate interfaces?
  • Take another completely different approach?
+6  A: 

The pure approach would be: Make everything an interface. As implementation details, you may optionally use any of various forms of composition or implementation-inheritance. Since these are implementation details, they don't matter to your public API, so you are free to choose whichever makes your life simplest.

Justice
Yes, and you can pick one implementation now and change your mind later without breaking other code.
Jason Cohen
+31  A: 

Mark, This is an interesting question. You will find as many opinions on this. I don't believe there is a 'right' answer. This is a great example of where a rigid heirarchial object design can really cause problems after a system is built.

For example, lets say you went with the "Customer" and "Staff" classes. You deploy your system and everything is happy. A few weeks later, someone points out that they are both 'on staff' and a 'customer' and they are not getting customer emails. In this case, you have a lot of code changes to make (re-design, not re-factor).

I believe it would be overly complex and difficult to maintain if you attempt to have a set of derived classes that implement all the permutations and combination of people and their roles. This is especially true given that the above example is very simple - in most real applications, things will be more complex.

For your example here, I would go with "Take another completely different approach". I would implement the Person class and include in it a collection of "roles". Each person could have one or more roles such as "Customer", "Staff", and "Vendor".

This will make it easier to add roles as new requirements are discovered. For example, you may simply have a base "Role" class, and derive new roles from them.

Foredecker
+14  A: 

You may want to consider using the Party and Accountability patterns

This way Person will have a collection of Accountabilities which may be of type Customer or Staff.

The model will also be simpler if you add more relationship types later.

Jorge Villuendas
A: 

We study this problem at college last year, we were learning eiffel so we used multiple inheritance. Anyway Foredecker roles alternative seems to be flexible enough.

+2  A: 

I would avoid the "is" check ("instanceof" in Java). One solution is to use a Decorator Pattern. You could create an EmailablePerson that decorates Person where EmailablePerson uses composition to hold a private instance of a Person and delegates all non-email methods to the Person object.

Paul Croarkin
A: 

Hi,

What's wrong in sending an email to a Customer who is a Staff member? If he is a customer, then he can be sent the email. Am I wrong in thinking so? And why should you take "everyone" as your email list? Woudlnt it be better to have a customer list since we are dealing with "sendEmailToCustomer" method and not "sendEmailToEveryone" method? Even if you want to use "everyone" list you cannot allow duplicates in that list.

If none of these are achievable with a lot of redisgn, I will go with the first Foredecker answer and may be you should have some roles assigned to each person.

vj01
In the given example, a Person cannot be both a customer and a staff member. That's what the question is about.
OregonGhost
Hi,I think the question is more about " I do not want to send multiple emails if a Person is both a Customer and a Staff member".To solve this issue, 1) "Everyone" should not allow duplicates2) If it allows duplicates then Person class should have "Roles" defined as pointed out by Foredecker
vj01
+3  A: 

Let me know if I understood Foredecker's answer correctly. Here's my code (in Python; sorry, I don't know C#). The only difference is I wouldn't notify something if a person "is a customer", I would do it if one of his role "is interested in" that thing. Is this flexible enough?

# --------- PERSON ----------------

class Person:
    def __init__(self, personId, name, dateOfBirth, address):
     self.personId = personId
     self.name = name
     self.dateOfBirth = dateOfBirth
     self.address = address
     self.roles = []

    def addRole(self, role):
     self.roles.append(role)

    def interestedIn(self, subject):
     for role in self.roles:
      if role.interestedIn(subject):
       return True
     return False

    def sendEmail(self, email):
     # send the email
     print "Sent email to", self.name

# --------- ROLE ----------------

NEW_DVDS = 1
NEW_SCHEDULE = 2

class Role:
    def __init__(self):
     self.interests = []

    def interestedIn(self, subject):
     return subject in self.interests

class CustomerRole(Role):
    def __init__(self, customerId, joinedDate):
     self.customerId = customerId
     self.joinedDate = joinedDate
     self.interests.append(NEW_DVDS)

class StaffRole(Role):
    def __init__(self, staffId, jobTitle):
     self.staffId = staffId
     self.jobTitle = jobTitle
     self.interests.append(NEW_SCHEDULE)

# --------- NOTIFY STUFF ----------------

def notifyNewDVDs(emailWithTitles):
    for person in persons:
     if person.interestedIn(NEW_DVDS):
      person.sendEmail(emailWithTitles)

Federico Ramponi
yes, this looks a nice solution and is very extensible.
Mark Heath
+1  A: 

Your classes are just data structures: none of them has any behaviour, just getters and setters. Inheritance is inappropriate here.

fizzer
+5  A: 

A Person is a human being, whereas a Customer is just a Role that a Person may adopt from time to time. Man and Woman would be candidates to inherit Person, but Customer is a different concept.

The Liskov substitution principle says that we must be able to use derived classes where we have references to a base class, without knowing about it. Having Customer inherit Person would violate this. A Customer might perhaps also be a role played by an Organisation.

A: 

Take another completely different approach: The problem with the class StaffCustomer is that your member of staff could start off as just staff and become a customer later on so you would need to delete them as staff and create a new instance of StaffCustomer class. Perhaps a simple boolean within Staff class of 'isCustomer' would allow our everyone list (presumably compiled from getting all customers and all staff from appropriate tables) to not get the staff member as it will know it has been included already as a customer.

Mike D