Object Oriented Best Practices - Inheritance v Composition v Interfaces

Question

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?

Answer 1

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.

Answer 2

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.

Answer 3

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.

Answer 4

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.

Answer 5

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)

Answer 6

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.

Answer 7

Here's some more tips: From the category of “do not even think to do this” here are some bad examples of code encountered:

Finder method returns Object

Problem: Depending on the number of occurrences found the finder method returns a number representing the number of occurrences – or! If only one found returns the actual object.

Don’t do this! This is one of the worst coding practices and it introduces ambiguity and messes the code in a way that when a different developer comes into play she or he will hate you for doing this.

Solution: If there’s a need for such 2 functionalities: counting and fetching an instance do create 2 methods one which returns the count and one which returns the instance, but never a single method doing both ways.

Problem: A derived bad practice is when a finder method returns either the one single occurrence found either an array of occurrences if more than one found. This lazy programming style is done alot by the programmers who do the previous one in general.

Solution: Having this on my hands I would return an array of length 1(one) if only one occurrence is found and an array with length >1 if more occurrences found. Moreover, finding no occurrences at all would return null or an array of length 0 depending on the application.

Programming to an interface and using covariant return types

Problem: Programming to an interface and using covariant return types and casting in the calling code.

Solution: Use instead the same supertype defined in the interface for defining the variable which should point to the returned value. This keeps the programming to an interface approach and your code clean.

Classes with more than 1000 lines are a lurking danger Methods with more than 100 lines are a lurking danger too!

Problem: Some developers stuff too much functionality in one class/method, being too lazy to break the functionality – this leads to low cohesion and maybe to high coupling – the inverse of a very important principle in OOP! Solution: Avoid using too much inner/nested classes – these classes are to be used ONLY on a per need basis, you don’t have to do a habit using them! Using them could lead to more problems like limiting inheritance. Lookout for code duplicate! The same or too similar code could already exist in some supertype implementation or maybe in another class. If it’s in another class which is not a supertype you also violated the cohesion rule. Watch out for static methods – maybe you need an utility class to add!
More at: http://centraladvisor.com/it/oop-what-are-the-best-practices-in-oop

Answer 8

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.

Answer 9

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.

Answer 10

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

Answer 11

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.

Answer 12

You probably don't want to use inheritance for this. Try this instead:

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

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

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