Is creating subclasses for specific instances a bad practice?

Question

Consider the following design

public class Person
{
    public virtual string Name { get; }

    public Person (string name)
    {
        this.Name = name;
    }
}

public class Karl : Person
{
    public override string Name
    {
        get
        {
            return "Karl";
        }
    }
}

public class John : Person
{
    public override string Name
    {
        get
        {
            return "John";
        }
    }
}

Do you think there is something wrong here? To me Karl and John classes should be just instances instead of classes as they are exactly the same as :

Person karl = new Person("Karl");
Person john = new Person("John");

Why would I create new classes when instances are enough? The classes does not add anything to the instance.

Answer 1

There is no need to have specific subclasses for every person.

You're right, those should be instances instead.

Goal of subclasses: to extend parent classes

Subclasses are used to extend the functionality provided by the parent class. For example, you may have:

• A parent class Battery which can Power() something and can have a Voltage property,

• And a subclass RechargeableBattery, which can inherits Power() and Voltage, but can also be Recharge()d.

Notice that you can pass an instance of RechargeableBattery class as a parameter to any method which accepts a Battery as an argument. This is called Liskov substitution principle, one of the five SOLID principles. Similarly, in real life, if my MP3 player accepts two AA batteries, I can substitute them by two rechargeable AA batteries.

Note that it is sometimes difficult to determine whether you need to use a field or a subclass to represent a difference between something. For example, if you have to handle AA, AAA and 9-Volt batteries, would you create three subclasses or use an enum? “Replace Subclass with Fields” in Refactoring by Martin Fowler, page 232, may give you some ideas and how to move from one to another.

In your example, Karl and John don't extend anything, nor do they provide any additional value: you can have the exactly same functionality using Person class directly. Having more lines of code with no additional value is never good.

An example of a business case

What could possibly be a business case where it would actually make sense to create a subclass for a specific person?

Let's say we build an application which manages persons working in a company. The application manages permissions too, so Helen, the accountant, cannot access SVN repository, but Thomas and Mary, two programmers, cannot access accounting-related documents.

Jimmy, the big boss (founder and CEO of the company) have very specific privileges no one has. He can, for example, shut down the entire system, or fire a person. You get the idea.

The poorest model for such application is to have classes such as:

         

because code duplication will arise very quickly. Even in the very basic example of four employees, you will duplicate code between Thomas and Mary classes. This will push you to create a common parent class Programmer. Since you may have multiple accountants as well, you will probably create Accountant class as well.

         

Now, you notice that having the class Helen is not very useful, as well as keeping Thomas and Mary: most of your code works at the upper level anyway—at the level of accountants, programmers and Jimmy. The SVN server doesn't care if it's Thomas or Mary who needs to access the log—it only needs to know whether it's a programmer or an accountant.

if (person is Programmer)
{
    this.AccessGranted = true;
}

So you end up removing the classes you don't use:

         

“But I can keep Jimmy as-is, since there would always be only one CEO, one big boss—Jimmy”, you think. Moreover, Jimmy is used a lot in your code, which actually looks more like this, and not as in the previous example:

if (person is Jimmy)
{
    this.GiveUnrestrictedAccess(); // Because Jimmy should do whatever he wants.
}
else if (person is Programmer)
{
    this.AccessGranted = true;
}

The problem with that approach is that Jimmy can still be hit by a bus, and there would be a new CEO. Or the board of directors may decide that Mary is so great that she should be a new CEO, and Jimmy would be demoted to a position of a salesperson, so now, you need to walk through all your code and change everything.

Answer 2

It's silly to use this kind of class structure just to vary details which should obviously be settable fields on an instance. But that is particular to your example.

Making different Persons different classes is almost certainly a bad idea - you'd have to change your program and recompile every time a new Person enters your domain. However, that doesn't mean that inheriting from an existing class so that a different string is returned somewhere isn't sometimes useful.

The difference is how you expect the entities represented by that class to vary. People are almost always assumed to come and go, and almost every serious application dealing with users is expected to be able to add and remove users at run-time. But if you model things like different encryption algorithms, supporting a new one is probably a major change anyway, and it makes sense to invent a new class whose myName() method returns a different string (and whose perform() method presumably does something different).

Answer 3

Why would I create new classes when instances are enough?

In most cases, you wouldn't. Your example is really a good case where this behaviour doesn't add real value.

It also violates the Open Closed principle, as the subclasses basically aren't an extension but modify the parent's inner workings. Moreover, the public constructor of the parent is now irritating in the subclasses and the API thus became less comprehensible.

However, somtimes, if you will only ever have one or two special configurations that are often used throughout the code, it is sometimes more convenient and less time consuming to just subclass a parent that has a complex constructor. In such special cases, I can see nothing wrong with such an approach. Let's call it constructor currying j/k

Answer 4

If this is the extent of the practice, then I agree this is a bad practice.

If John and Karl have different behaviors, then things change a little bit. It could be that person has a method for cleanRoom(Room room) where it turns out that John is a great roommate and cleans very effectively, but Karl is not and doesn't clean the room very much.

In this case, it would make sense to have them be their own subclass with the behavior defined. There are better ways to achieve the same thing (for example, a CleaningBehavior class), but at least this way isn't a terrible violation of OO principles.

Answer 5

In some cases you know there will only be a set number of instances and then it would kind of OK (although ugly in my opinion) to use this approach. It is better to use an enum if the language allows it.

Java example:

public enum Person
{
    KARL("Karl"),
    JOHN("John")

    private final String name;

    private Person(String name)
    {
        this.name = name;
    }

    public String getName()
    {
        return this.name;
    }
}

Answer 6

A lot of people answered already. Thought I'd give my own personal perspective.

---

Once upon a time I worked on an app (and still do) that creates music.

The app had an abstract Scale class with several subclasses: CMajor, DMinor, etc. Scale looked something like so:

public abstract class Scale {
    protected Note[] notes;
    public Scale() {
        loadNotes();
    }
    // .. some other stuff ommited
    protected abstract void loadNotes(); /* subclasses put notes in the array
                                          in this method. */
}

The music generators worked with a specific Scale instance to generate music. The user would select a scale from a list, to generate music from.

One day, a cool idea came to my mind: why not allow the user to create his/her own scales? The user would select notes from a list, press a button, and a new scale would be added to the list available scales.

But I wasn't able to do this. That was because all the scales are already set at compile time - since they are expressed as classes. Then it hit me:

It's often intuitive to think in terms of 'superclasses and subclasses'. Almost everything can be expressed through this system: superclass Person and subclasses John and Mary; superclass Car and subclasses Volvo and Mazda; superclass Missile and subclasses SpeedRocked, LandMine and TrippleExplodingThingy.

It's very natural to think in this way, especially for the person relatively new to OO.

But we should always remember that classes are templates, and objects are content poured into these templates. You can pour whatever content you want into the template, creating countless possibilities.

It's not the job of the subclass to fill the template. It's the job of the object. The job of the subclass is to add actual functionality, or expand the template.

And that's why I should have created a concrete Scale class, with a Note[] field, and let objects fill in this template; possibly through the constructor or something. And eventually, so I did.

Every time you design a template in a class (e.g., an empty Note[] member that needs to be filled, or a String name field that needs to be assigned a value), remember that it's the job of the objects of this class to fill in the template (or possibly those creating these objects). Subclasses are meant to add functionality, not to fill in templates.

---

You might be tempted to create a "superclass Person, subclasses John and Mary" kind of system, like you did, because you like the formality this gets you.

This way, you can just say Person p = new Mary(), instead of Person p = new Person("Mary", 57, Sex.FEMALE). It makes things more organized, and more structured. But as we said, creating a new class for every combination of data isn't good approach, since it bloats the code for nothing and limits you in terms of runtime abilities.

So here's a solution: use a basic factory, might even be a static one. Like so:

public final class PersonFactory {
    private PersonFactory() { }

    public static Person createJohn(){
        return new Person("John", 40, Sex.MALE);
    }

    public static Person createMary(){
        return new Person("Mary", 57, Sex.FEMALE);
    }

    // ...
}

This way, you can easily use the 'presets' the 'come with the program', like so: Person mary = PersonFactory.createMary(), but you also reserve the right to design new persons dynamically, for example in the case that you want to allow the user to do so. E.g.:

// .. requesting the user for input ..
String name = // user input
int age = // user input
Sex sex = // user input, interpreted
Person newPerson = new Person(name, age, sex);

Or even better: do something like so:

public final class PersonFactory {
    private PersonFactory() { }

    private static Map<String, Person> persons = new HashMap<>();
    private static Map<String, PersonData> personBlueprints = new HashMap<>();

    public static void addPerson(Person person){
        persons.put(person.getName(), person);
    }

    public static Person getPerson(String name){
        return persons.get(name);
    }

    public static Person createPerson(String blueprintName){
        PersonData data = personBlueprints.get(blueprintName);
        return new Person(data.name, data.age, data.sex);
    }

    // .. or, alternative to the last method
    public static Person createPerson(String personName){
        Person blueprint = persons.get(personName);
        return new Person(blueprint.getName(), blueprint.getAge(), blueprint.getSex());
    }

}

public class PersonData {
    public String name;
    public int age;
    public Sex sex;

    public PersonData(String name, int age, Sex sex){
        this.name = name;
        this.age = age;
        this.sex = sex;
    }
}

I got carried away. I think you get the idea.

---

Subclasses are not meant to fill in the templates set by their superclasses. Subclasses are meant to add functionality. Object are meant to fill in the templates, that's what they're for.

You shouldn't be creating a new class for every possible combination of data. (Just like I shouldn't have created a new Scale subclass for every possible combination of Notes).

Her'es a guideline: whenever you create a new subclass, consider if it adds any new functionality that doesn't exist in the superclass. If the answer to that question is "no", than you might be trying to 'fill in the template' of the superclass, in which case just create an object. (And possibly a Factory with 'presets', to make life easier).

Hope that helps.

Answer 7

This is an exception to the 'should be instances' here - although slightly extreme.

If you wanted the compiler to enforce permissions to access methods based on whether it is Karl or John (in this example) rather than ask the method implementation to check for which instance has been passed then you might use different classes. By enforcing different classes rather than instantiation then you make differentiation checks between 'Karl' and 'John' possible at compile time rather than run time and this might be useful - particularly in a security context where the compiler may be more trusted than run-time code checks of (for example) external libraries.

Answer 8

It's considered a bad practice to have duplicated code.

This is at least partially because lines of codes and therefore size of codebase correlates to maintenance costs and defects.

Here's the relevant common sense logic to me on this particular topic :

1) If I were to need to change this, how many places would I need to visit? Less is better here.

2) Is there a reason why it is done this way? In your example - there couldn't be - but in some examples there might be some kind of reason for doing this. One I can think of off the top of my head is in some frameworks like early Grails where inheritance didn't necessarily play nicely with some of the plugins for GORM. Few and far between.

3) Is it cleaner and easier to understand this way? Again, it isn't in your example - but there are some inheritance patterns that may be more of a stretch where separated classes is actually easier to maintain (certain usages of the command or strategy patterns come to mind).

Answer 9

There is a use case: forming a reference to a function or method as a regular object.

You can see this in Java GUI code a lot:

ActionListener a = new ActionListener() {
    public void actionPerformed(ActionEvent arg0) {
        /* ... */
    }
};

The compiler helps you here by creating a new anonymous subclass.