Take the 2-minute tour ×
Programmers Stack Exchange is a question and answer site for professional programmers interested in conceptual questions about software development. It's 100% free, no registration required.

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.

share|improve this question
4  
Unfortunately, you will find this in lots of production code. Clean it up when you can. –  Adam Zuckerman 9 hours ago
2  
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 6 hours ago
    
Here's a sort of related question where the OP seems to believe the opposite of conventional wisdom. programmers.stackexchange.com/questions/253612/… –  Dunk 5 hours ago

7 Answers 7

up vote 29 down vote accepted

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:

           Person
┎━━━━┰━┸━━┰━━━┒
Helen   Thomas   Mary   Jimmy

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.

                  Person
    ┎━━━━━━━━╁━━━━━━━┒
Accountant      Programmer       Jimmy
    ┃         ┎━━┸━━┒
   Helen     Thomas      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:

               Person
    ┎━━━━━━╁━━━━━┒
Accountant   Programmer   Jimmy

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

share|improve this answer
3  
This answer is surely correct, but so many upvotes without any explanation? –  Doc Brown 6 hours ago
3  
@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. –  MainMa 6 hours ago
1  
Short answers tend to be posted first. Earlier posts get more up votes. –  Tim B 5 hours ago
    
@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. –  MainMa 5 hours ago
    
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 1 hour ago

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

share|improve this answer

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

share|improve this answer
    
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 7 hours ago
    
@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 7 hours ago
    
So any use of virtual members is an OCP violation? –  Ben Aaronson 7 hours ago
    
@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. –  Cory 3 hours ago

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.

share|improve this answer

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;
    }
}
share|improve this answer

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.

share|improve this answer

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

share|improve this answer

Your Answer

 
discard

By posting your answer, you agree to the privacy policy and terms of service.

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