37

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.

  • 17
    Unfortunately, you will find this in lots of production code. Clean it up when you can. – Adam Zuckerman Sep 29 '14 at 15:22
  • 14
    This is a great design - if a developer wants to make himself indispensable for every change in business data, and has no problems to be available 24/7 ;-) – Doc Brown Sep 29 '14 at 17:42
  • 1
    Here's a sort of related question where the OP seems to believe the opposite of conventional wisdom. programmers.stackexchange.com/questions/253612/… – Dunk Sep 29 '14 at 19:18
  • 11
    Design your classes depending on behavior not data. To me the sub-classes add no new behavior to the hierarchy. – Songo Sep 30 '14 at 13:04
  • 2
    This is widely used with anonymous subclasses (such as event handlers) in Java before lambdas came to Java 8 (which is the same thing with different syntax). – marczellm Sep 30 '14 at 13:32
77

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:

          The class diagram shows Helen, Thomas, Mary and Jimmy classes and their common parent: the abstract Person class.

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.

          The class diagram shows an abstract Person with three children: abstract Accountant, an abstract Programmer, and Jimmy. Accountant has a subclass Helen, and Programmer has two subclasses: Thomas and Mary.

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:

          The class diagram contains Accountant, Programmer and Jimmy subclasses with its common parent class: abstract Person.

“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.

  • 6
    @DocBrown: I'm often surprised to see that short answers which are not wrong but not deep enough have a much higher score than answers which explain in depth the subject. Probably too many people don't like to read a lot, so very short answers look more attractive to them. Still, I edited my answer to provide at least some explanation. – Arseni Mourzenko Sep 29 '14 at 17:50
  • 3
    Short answers tend to be posted first. Earlier posts get more up votes. – Tim B Sep 29 '14 at 18:42
  • 1
    @TimB: I usually start with medium-sized answers, and then edit them to be very complete (here's an example, given that there was probably around fifteen revisions, only four being shown). I noticed that the longer becomes the answer over time, the less upvotes it receives; a similar question which remains short attracts more upvotes. – Arseni Mourzenko Sep 29 '14 at 18:58
  • Agree with @DocBrown. For example, a good answer would say something like "subclass for behavior, not for content", and refer to some reference that I won't do in this comment. – user949300 Sep 29 '14 at 22:38
  • 2
    In case it wasn't clear from the last paragraph: Even if you only have 1 person with unrestricted access, you should still make that person be an instance of a separate class that implements unrestricted access. Checking for the person itself results in code-data interaction and can open up a whole can of worms. For example, what if the Board of Directors decides to appoint a triumvirate as CEO? If you didn't have a class "Unrestricted", you wouldn't just have to update the existing CEO, but also add 2 more checks for the other members. If you did have it, you just set them as "Unrestricted". – Nzall Sep 30 '14 at 9:40
15

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).

12

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

  • I'm not sure I agree with the OCP paragraph. If a class is exposing a property setter to its descendants then- assuming it's following good design principles- the property should not be part of its inner workings – Ben Aaronson Sep 29 '14 at 16:48
  • @BenAaronson As long as the behaviour of the property itself isn't altered, you don't violate OCP, but here they do. Of course you can use that Property in its inner workings. But you shouldn't alter existing behaviour! You should only extend behaviour, not change it with inheritance. Of course, there're exceptions to every rule. – Falcon Sep 29 '14 at 17:03
  • 1
    So any use of virtual members is an OCP violation? – Ben Aaronson Sep 29 '14 at 17:13
  • 3
    @Falcon There's no need to derive a new class just to avoid repetitive complex constructor calls. Just create factories for the common-cases and parameterize the small variations. – Keen Sep 29 '14 at 20:49
  • @BenAaronson I'd say having virtual members that have an actual implementation is such a violation. Abstract members, or say methods that do nothing, would be valid extension points. Basically, when you look at the code of a base class, you know that it's going to run as-is, instead of every non-final method being a candidate for being replaced with something entirely different. Or to put it differently: the ways by which an inheriting class can affect the behaviour of the base class will be explicit, and constrained to being "additive". – millimoose Oct 1 '14 at 0:31
8

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.

6

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;
    }
}
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.

  • This is an excellent example, thank you very much. – Ignacio Soler Garcia Oct 1 '14 at 15:42
  • @SoMoS Glad I could help. – Aviv Cohn Oct 1 '14 at 17:48
2

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.

2

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).

  • 1
    whether it's bad or not is not set in stone. There are for example scenarios where the extra overhead of object creation and method calls can be so detrimental for performance that the application no longer meets specifications. – jwenting Sep 30 '14 at 7:05
  • See point #2. Sure there's reasons why, occasionally. That's why you need to ask before gutting out someone's code. – dcgregorya Sep 30 '14 at 19:22
0

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.

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service, privacy policy and cookie policy

Not the answer you're looking for? Browse other questions tagged or ask your own question.