Declaring children type in base class; Is it bad or not?

Question

Recently I came across some code that has declared the children types as an enumeration in the base class. Here's a simple example:

public enum EmployeeType
{
    Manager,
    Secretary
}

public class Employee
{
    public string Code { get; set; }

    public EmployeeType Type { get; set; }
}

public class Manager : Employee
{
    public void Manage()
    {
        // Managing
    }
}

public class Secretary : Employee
{
    public void SetMeeting()
    {
        // Setting meeting
    }
}

Based on my development experience, I wrote an article about it, declaring that it's a bad practice/design. I think it's bad because the base class should be agnostic about its children classes. It should have no information about its children classes, and here are at least two reasons:

1. Extensibility: This design won't be extensible, because if you want to define another derived class, say Developer for example, you should also update the EmployeeType enumeration, to which you might not have access.

2. Paradoxical definition: Now you can write this code:

   Secretary secretary = new Secretary();
   secretary.EmployeeType = EmployeeType.Manager;
   /*
   This is absurd semantically. But syntactically, it's possible.
   */
   

However, as I read Wikipedia's article about inheritance, I couldn't find any answer to my question.

While it might sound arguable at first glance, I believe that inheritance should be mature enough to have a solid answer for this dilemma. Is this code bad, smelly code? Or is it acceptable and justified? Why?

Answer 1

I think the problem can be solved when we return to the philosophy of when to use Inheritance.
It is said that "Think of inheritance as an is a relationship"

When you establish an inheritance relationship between two classes, you get to take advantage of dynamic binding and polymorphism.
Dynamic binding means the at runtime which method implementation to invoke, based on the class of the object.
Polymorphism means you can use a variable of a superclass type to hold a reference to an object whose class is the superclass or any of its subclasses.

One of the prime benefits of dynamic binding and polymorphism is that they can help make code easier to change. If you have a fragment of code that uses a variable of a superclass type, such as Employee, you could later create a brand new subclass, such as Manager, and the old code fragment will work without change with instances of the new subclass. If Manager overrides any of Employee's methods that are invoked by the code fragment, dynamic binding will ensure that Managers's implementation of those methods gets executed. This will be true even though class Manager didn't exist when the code fragment was written and compiled.

Now back to the question, why we need a EmployeeType in Employee?
There may be functions that we would like to implement in Employee, AKA CalculateSalary()

CalculateSalary(){
if (EmployeeType == EmployeeType.Manager) // Add management percent }

Tempting reason but what if the Employee is both a Manager and a Supervisor?
Its going to be complicated! Simply we can implement CalculateSalary() as an abstract method but we will lose the target!
Here comes the Coad's Rule:
A subclass expresses is a special kind of and not is a role played by a.
Liskov Substitution Principle is a test for 'Should I be inheriting from this type?'

Role of the thumb for the above is:

• Does TypeB want to expose the complete interface (all public methods no less) of TypeA such that TypeB can be used where TypeA is expected? Indicates Inheritance.
  e.g. A Cessna biplane will expose the complete interface of an airplane, if not more. So that makes it fit to derive from Airplane.
• Does TypeB only want only some/part of the behavior exposed by TypeA? Indicates need for Composition.
  e.g. A Bird may need only the fly behavior of an Airplane. In this case, it makes sense to extract it out as an interface / class / both and make it a member of both classes.
  

As a foot note from javaworld:
Make sure inheritance models the is-a relationship.
Don't use inheritance just to get code reuse.
Don't use inheritance just to get at polymorphism.
Favor composition Over Inheritance.

I think in scenarios like this, being a manager is a role played by the employee, that role may be changed over time, the role would be implemented as a composition.

As an another aspect of view in domain model, and mapping it to a relational database. It's really hard to map inheritance to the SQL model (you end up with creating columns that aren't always used, using views, etc). Some ORMs try to deal with this, but it always gets complicated quickly.
For example Hibernate's One Table Per Class Hierarchy approach, that violates 2NF and could cause inconsistency very likely to your sample of

Secretary secretary = new Secretary();
secretary.EmployeeType = EmployeeType.Manager;

Answer 2

It is definitely bad practice. Leaving aside the dubious use of inheritance (which is in itself widely viewed as bad practice, compared with using composition), it breaks the Dependency inversion principle by introducing a nasty circular dependency between parent and child classes.

In redesigning the code, I'd expect to address three issues:

1. I'd remove the circular dependency, by removing the enum from the base class.
2. I'd make Employee an interface.
3. I'd look at removing the public setters, which allow any aspect of the application to modify core aspects of the employee's record, making testing and tracking down bugs far more difficult than they need be.