37

I was having a discussion with a co-worker, and we ended up having conflicting intuitions about the purpose of subclassing. My intuition is that if a primary function of a subclass is to express a limited range of possible values of its parent, then it probably shouldn't be a subclass. He argued for the opposite intuition: that subclassing represents an object's being more "specific", and therefore a subclass relationship is more appropriate.

To put my intuition more concretely, I think that if I have a subclass that extends a parent class but the only code that subclass overrides is a constructor (yes, I know constructors don't generally "override", bear with me), then what was really needed was a helper method.

For example, consider this somewhat real-life class:

public class DataHelperBuilder
{
    public string DatabaseEngine { get; set; }
    public string ConnectionString { get; set; }

    public DataHelperBuilder(string databaseEngine, string connectionString)
    {
        DatabaseEngine = databaseEngine;
        ConnectionString = connectionString;
    }

    // Other optional "DataHelper" configuration settings omitted

    public DataHelper CreateDataHelper()
    {
        Type dataHelperType = DatabaseEngineTypeHelper.GetType(DatabaseEngine);
        DataHelper dh = (DataHelper)Activator.CreateInstance(dataHelperType);
        dh.SetConnectionString(ConnectionString);

        // Omitted some code that applies decorators to the returned object
        // based on omitted configuration settings

        return dh;
    }
}

His claim is that it would be entirely appropriate to have a subclass like this:

public class SystemDataHelperBuilder
{
    public SystemDataHelperBuilder()
        : base(Configuration.GetSystemDatabaseEngine(),
               Configuration.GetSystemConnectionString())
    {
    }
 }

So, the question:

  1. Among people who talk about design patterns, which of these intuitions is correct? Is subclassing as described above an anti-pattern?
  2. If it is an anti-pattern, what is its name?

I apologize if this turns out to have been an easily googleable answer; my searches on google mostly returned information about the telescoping constructor anti-pattern and not really what I was looking for.

We're looking for long answers that provide some explanation and context. Don't just give a one-line answer; explain why your answer is right, ideally with citations. Answers that don't include explanations may be removed.

  • 2
    I consider the word "Helper" in a name to be an antipattern. It's like reading an essay with constant references to a thingie and a whatsit. Say what it is. Is it a factory? Constructor? To me it smacks of a lazy thought process, and it encourages violation of the single responsibility principle, since a proper semantic name would direct it. I agree with the other voters, and you should accept @lxrec's answer. Creating a subclass for a factory method is completely pointless, unless you can't trust users to pass correct arguments as specified in documentation. In which case, get new users. – Aaron Hall Feb 28 '15 at 13:46
  • I absolutely agree with @Ixrec's answer. After all, his answer says that I was right all along and my co-worker was wrong. ;-) But seriously, that's exactly why I felt weird accepting it; I thought I could be accused of bias. But I'm new to the programmers StackExchange; I'd be willing to accept it if I was assured it wouldn't be a breach of etiquette. – HardlyKnowEm Mar 2 '15 at 16:15
  • 1
    No, it is expected for you to accept the answer you think is most valuable. The one the community thinks is most valuable so far is Ixrec's by far by more than 3 to 1. Thus they would expect you to accept it. In this community, there is very little frustration expressed in a questioner accepting the wrong answer, but there is a great deal of frustration expressed in questioners that never accept an answer. Note that no one is complaining about the accepted answer here, instead they complain about the voting, which appears to be righting itself: stackoverflow.com/q/2052390/541136 – Aaron Hall Mar 2 '15 at 16:25
  • Very well, I'll accept it. Thank you for taking the time to educate me about the community standards here. – HardlyKnowEm Mar 2 '15 at 17:51

11 Answers 11

54

If all you want to do is create class X with certain arguments, subclassing is an odd way of expressing that intent, because you aren't using any of the features that classes and inheritance give you. It's not really an anti-pattern, it's just strange and a bit pointless (unless you have some other reasons for it). A more natural way of expressing this intent would be a Factory Method, which in this case is a fancy name for your "helper method."

Regarding the general intuition, both "more specific" and "a limited range" are potentially harmful ways of thinking about subclasses, because they both imply that making Square a subclass of Rectangle is a good idea. Without relying on something formal like LSP, I would say a better intuition is that a subclass either provides an implementation of the base interface, or extends the interface to add some new functionality.

  • 2
    Factory Methods though don't let you enforce certain behaviors (governed by arguments) in your codebase. And sometimes it's useful to explicitly annotate that this class/method/field takes SystemDataHelperBuilder specifically. – Telastyn Feb 26 '15 at 19:49
  • 3
    Ah, the square vs. rectangle argument was exactly what I was looking for, I think. Thanks! – HardlyKnowEm Feb 26 '15 at 19:54
  • 7
    Of course, having square be a subclass of rectangle can be okay if they're immutable. You really have to look to the LSP. – David Conrad Feb 26 '15 at 21:45
  • 10
    The thing about the square/rectangle "problem" is that geometric shapes are immutable. You can always use a square wherever a rectangle makes sense, but resizing is not something squares or rectangles do. – Doval Feb 27 '15 at 3:33
  • 3
    @nha LSP = Liskov Substitution Principle – Ixrec Feb 27 '15 at 19:44
16

Which of these intuitions is correct?

Your coworker is correct (assuming standard type systems).

Think about it, classes represent possible legal values. If class A has one byte field F, you might be inclined to think that A has 256 legal values, but that intuition is incorrect. A is restricting "every permutation of values ever" to "must have field F be 0-255".

If you extend A with B which has another byte field FF, that is adding restrictions. Instead of "value where F is byte", you have "value where F is byte && FF is also byte". Since you're keeping the old rules, everything that worked for the basetype still works for your subtype. But since you're adding rules, you're further specializing away from "could be anything".

Or to think of it another way, assume that A had a bunch of subtypes: B, C, and D. A variable of type A could be any of these subtypes, but a variable of type B is more specific. It (in most type systems) cannot be a C or a D.

Is this an anti-pattern?

Enh? Having specialized objects is useful and not clearly detrimental to code. Achieving that result by using subtyping is perhaps a little questionable, but depends on what tools you have at your disposal. Even with better tools this implementation is simple, straightforward and robust.

I would not consider this an anti-pattern since it is not clearly wrong and bad in any situation.

  • 5
    "More rules means fewer possible values, means more specific." I really don't follow this. Type byte and byte definitely has more values than just type byte; 256 times as many. That aside, I don't think you can conflate rules with number of elements (or cardinality if you want to be precise). It breaks down when you consider infinite sets. There's just as many even numbers as there are integers, but knowing that a number is even is still a restriction/gives me more information than only knowing it's an integer! Same could be said for the natural numbers. – Doval Feb 26 '15 at 19:59
  • 6
    @Telastyn We're getting off-topic, but it's provable that cardinality (loosely, "size") of the set of even integers is the same as the set of all integers, or even all rational numbers. It's really cool stuff. See math.grinnell.edu/~miletijo/museum/infinite.html – HardlyKnowEm Feb 26 '15 at 20:14
  • 2
    Set theory is pretty unintuitive. You can put the integers and evens in one-to-one correspondence (e.g. using the function n -> 2n). This not always possible; you can't map the integers to the reals, so the set of reals is "bigger" than the integers. But you can map the (real) interval [0, 1] to the set of all reals, so that makes them the same "size". Subsets are defined as "every element of A is also an element of B" precisely because once your sets get infinite, it can be the case that they're the same cardinality but have different elements. – Doval Feb 26 '15 at 20:14
  • 1
    More related to the topic at hand, the example you give is a constraint that, in terms of object-oriented programming, actually extends -functionality- in terms of an additional field. I'm talking about subclasses that don't extend functionality--like the circle-ellipse problem. – HardlyKnowEm Feb 26 '15 at 20:17
  • 1
    @Doval: There is more than one possible meaning of "fewer" - i.e. more than one ordering relation on sets. The "is a subset of" relation gives a well-defined (partial) ordering on sets. Moreover, "more rules" can be taken to mean "all elements of the set satisfy more (i.e. a superset of) propositions (from a certain set)". With these definitions, "more rules" does indeed mean "fewer values". This is, very roughly speaking, the approach taken by a number of semantic models of computer programs, eg Scott domains. – psmears Feb 26 '15 at 23:42
12

No, it's not an anti-pattern. I can think of a number of practical use cases for this:

  1. If you want to use compile time checking to make sure collections of objects only conform to one particular subclass. For example, if you have MySQLDao and SqliteDao in your system, but for some reason you want to make sure a collection only contains data from one source, if you use subclasses as you describe you can have the compiler verify this particular correctness. On the other hand, if you store the data source as a field, this would become a run-time check.
  2. My current application has a one to one relationship between AlgorithmConfiguration + ProductClass and AlgorithmInstance. In other words, if you configure FooAlgorithm for ProductClass in the properties file, you can only get one FooAlgorithm for that product class. The only way to get two FooAlgorithms for a given ProductClass is to create a subclass FooBarAlgorithm. This is okay, because it makes the properties file easy to use (there is no need for syntax for many different configurations in one section in the properties file), and it's very rare there will be more than one instance for a given product class.
  3. @Telastyn's answer gives another good example.

Bottom line: There isn't really any harm in doing this. An Anti-pattern is defined to be:

An anti-pattern (or antipattern) is a common response to a recurring problem that is usually ineffective and risks being highly counterproductive.

There is no risk of being highly counterproductive here, so it's not an anti-pattern.

  • 2
    Yeah, I think if I had to phrase the question over again, I'd say "code smell." It's not bad enough to warrant warning the next generation of young developers to avoid it, but it's something that if you see it, it may indicate that there is issue in the overarching design, but may also ultimately be correct. – HardlyKnowEm Feb 27 '15 at 20:27
  • Point 1 is right on target. I didn't really understand point 2, but I'm sure it's just as good... :) – Phil Mar 4 '15 at 1:24
9

If something is a pattern or an antipattern depends significantly on what language and environment you are writing in. For example, for loops are a pattern in assembly, C, and similar languages and an anti-pattern in lisp.

Let me tell you a story of some code that I wrote long ago...

It was in a language called LPC and implemented a framework for casting spells in a game. You had a spell superclass, which had a subclass of combat spells that handled some checks, and then a subclass for single target direct damage spells, which was then subclassed to the individual spells - magic missile, lightning bolt and the like.

The nature of how LPC worked was that you had a master object that was a Singleton. before you go 'eww Singleton' - it was and it wasn't "eww" at all. One didn't make copies of the spells, but rather invoked the (effectively) static methods in the spells. The code for magic missile looked like:

inherit "spells/direct";

void init() {
  ::init();
  damage = 10;
  cost = 3;
  delay = 1.0;
  caster_message = "You cast a magic missile at ${target}";
  target_message = "You are hit with a magic missile cast by ${caster}";
}

And you see that? Its a constructor only class. It set some values that were in its parent abstract class (no, it shouldn't use setters - its an immutable) and that was it. It worked right. It was easy to code, easy to extend and easy to understand.

This sort of pattern translates to other situations where you have a subclass that extends an abstract class and sets a few values of its own, but all of the functionality is in the abstract class it inherits. Go glance at StringBuilder in Java for an example of this - not quite constructor only, but I'm pressed to find much logic in the methods themselves (everything is in the AbstractStringBuilder).

This is not a bad thing, and it is certainly not an anti-pattern (and in some languages it may be a pattern itself).

2

The most common use of such subclasses that I can think of is exception hierarchies, although that's a degenerate case where we would typically define nothing at all in the class, as long as the language lets us inherit constructors. We use inheritance to express that ReadError is a special case of IOError, and so on, but ReadError need not override any methods of IOError.

We do this because exceptions are caught by checking their type. Thus, we need to encode the specialisation in the type so that someone can catch only ReadError without catching all IOError, should they want to.

Normally, checking the types of things is terribly bad form and we try to avoid it. If we succeed in avoiding it, then there's no essential need to express specialisations in the type system.

It might still be useful though, for example if the name of the type appears in the logged string form of the object: this is language and possibly framework specific. You could in principle replace the name of the type in any such system with a call to a method of the class, in which case we would override more than just the constructor in SystemDataHelperBuilder, we'd also override loggingname(). So if there's any such logging in your system you could imagine that although your class literally only overrides the constructor, "morally" it's also overriding the class name, and so for practical purposes it isn't a constructor-only subclass. It has desirable different behaviour, it's just that you get this behaviour for free without writing any method overrides thanks to cunning use of reflection elsewhere.

So, I would say that his code can be a good idea if there's some code elsewhere that somehow checks for instances of SystemDataHelperBuilder, either explicitly with a branch (like exception catching branches based on type) or perhaps because there's some generic code that will end up using the name SystemDataHelperBuilder in a useful way (even if it's just logging).

However, using types for special cases does lead to some confusion, since if ImmutableRectangle(1,1) and ImmutableSquare(1) behave differently in any way then eventually someone is going to wonder why and perhaps wish it didn't. Unlike exception hierarchies, you check whether a rectangle is a square by checking whether height == width, not with instanceof. In your example, there's a difference between a SystemDataHelperBuilder and a DataHelperBuilder created with the exact same arguments, and that has potential to cause problems. Therefore, a function to return an object created with the "right" arguments seems to me normally the right thing to do: the subclass results in two different representations of "the same" thing, and it only helps if you're doing something you normally would try not to do.

Note that I'm not talking in terms of design patterns and anti-patterns, because I don't believe that it's possible to provide a taxonomy of all good and bad ideas that a programmer has ever thought of. Thus, not every idea is a pattern or anti-pattern, it only becomes that when someone identifies that it occurs repeatedly in different contexts and names it ;-) I'm not aware of any particular name for this kind of subclassing.

  • I could see uses for ImmutableSquareMatrix:ImmutableMatrix, provided there was an efficient means of asking an ImmutableMatrix to render its contents as an ImmutableSquareMatrix if possible. Even if ImmutableSquareMatrix was basically an ImmutableMatrix whose constructor required that height and width match, and whose AsSquareMatrix method would simply return itself (rather than e.g. constructing an ImmutableSquareMatrix which wraps the same backing array), such a design would allow compile-time parameter validation for methods which require square matrices – supercat Feb 27 '15 at 14:10
  • @supercat: good point, and in the questioner's example if there are functions that must be passed a SystemDataHelperBuilder, and not just any DataHelperBuilder will do, then it makes sense to define a type for that (and an interface, assuming you handle DI that way). In your example, there are some operations a square matrix could have that a non-square wouldn't, such as determinant, but your point is good that even if the system doesn't need those you'd still want to check by type. – Steve Jessop Feb 27 '15 at 16:15
  • ... so I suppose it's not entirely true what I said, that "you check whether a rectangle is a square by checking whether height == width, not with instanceof". You might prefer something you can assert statically. – Steve Jessop Feb 27 '15 at 16:19
  • I wish there were a mechanism which would allow something like constructor syntax to invoke static factory methods. The type of the expression foo=new ImmutableMatrix(someReadableMatrix) is clearer than that of someReadableMatrix.AsImmutable() or even ImmutableMatrix.Create(someReadableMatrix), but the latter forms offer useful semantic possibilities the former lacks; if the constructor can tell that the matrix is square, being able to have its type reflect that could be cleaner than requiring code downstream that needs a square matrix to make a new object instance. – supercat Feb 27 '15 at 16:51
  • @supercat: One small thing about Python that I like is the absence of a new operator. A class is just a callable, that when called happens to return (by default) an instance of itself. So if you want to preserve interface consistency it's perfectly possible to write a factory function whose name starts with a capital letter, therefore it looks just like a constructor call. Not that I do this routinely with factory functions, but it's there when you need it. – Steve Jessop Feb 27 '15 at 20:26
2

The strictest Object Oriented definition of a subclass relationship is known as «is-a». Using the traditional example of a square and a rectangle, a square «is-a» rectangle.

By this, you should use subclassing for any situation where you feel a «is-a» is a meaningful relationship for your code.

In your specific case of a subclass with nothing but a constructor, you should analyze it from a «is-a» perspective. It is clear that a SystemDataHelperBuilder «is-a» DataHelperBuilder, so the first pass suggests it is a valid use.

The question that you should answer is whether any of the other code leverages this «is-a» relationship. Does any other code attempt to distinguish SystemDataHelperBuilders from the general population of DataHelperBuilders? If so, the relationship is quite reasonable, and you should keep the subclass.

However, if no other code does this, then what you have is a relationshp that could be a «is-a» relationship, or it could be implemented with a factory. In this case, you could go either way, but I would recommend a Factory rather than a «is-a» relationship. When analyzing Object Oriented code written by another individual, the class hierarchy is a major source of information. It provides the «is-a» relationships they will need to make sense of the code. Accordingly, putting this behavior into a subclass promotes the behavior from "something someone will find if they dig into the superclass's methods" to "something everyone will look through before they even start diving into the code." Quoting Guido van Rossum, "code is read more often than it is written," so decreasing readability of your code for upcoming developers is a stiff price to pay. I would elect not to pay it, if I could use a factory instead.

1

A subtype is never "wrong" as long as you can always replace an instance of its supertype with an instance of the subtype and have everything still work correctly. This checks out as long as the subclass never tries to weaken any of the guarantees the supertype makes. It can make stronger (more specific) guarantees, so in that sense your coworker's intuition is correct.

Given that, the subclass you made probably isn't wrong (since I don't know exactly what they do, I can't say for sure.) You need to be wary of the fragile base class problem, and you also have to consider whether an interface would benefit you, but that's true of all uses of inheritance.

1

I think the key to answering this question is to look at this one, particular usage scenario.

Inheritance is used to enforce database configuration options, like the connection string.

This is an anti pattern because the class is violating the Single Responsibility Principal --- It is configuring itself with a specific source of that configuration and not "Doing one thing, and doing it well." The configuration of a class should not be baked into the class itself. For setting database connection strings, most times I've seen this happen at the application level during some sort of event occurring as the application is started up.

1

I use constructor only subclasses quite regularly to express different concepts.

For example, consider the following class:

public class Outputter
{
    private readonly IOutputMethod outputMethod;
    private readonly IDataMassager dataMassager;

    public Outputter(IOutputMethod outputMethod, IDataMassager dataMassager)
    {
        this.outputMethod = outputMethod;
        this.dataMassager = dataMassager;
    }

    public MassageDataAndOutput(string data)
    {
        this.outputMethod.Output(this.dataMassager.Massage(data));
    }
}

We can then create a subclass:

public class CapitalizeAndPrintOutputter : Outputter
{
    public CapitalizeAndPrintOutputter()
        : base(new PrintOutputMethod(), new Capitalizer())
    {
    }
}

You can then use a CapitalizeAndPrintOutputter anywhere in your code and you know exactly what type of outputter it is. Furthermore, this pattern actually makes it very easy to use a DI container to create this class. If the DI container knows about PrintOutputMethod and Capitalizer it can even automatically create the CapitalizeAndPrintOutputter for you.

Using this pattern is really quite flexible and useful. Yes, you can use factory methods to do the same thing, but then (at least in C#) you lose some of the power of the type system and certainly using DI containers becomes more cumbersome.

1

Let me first note that as the code is written, the sub-class is not actually more specific that the parent, as the two fields initialized are both settable by client code.

An instance of the subclass can only be distinguished using a downcast.

Now, if you have a design in which the DatabaseEngine and ConnectionString properties were reimplemented by the subclass, which accessed Configuration to do so, then you have a constraint which makes more sense of having the subclass.

Having said that, "anti-pattern" is too strong for my taste; there is nothing really wrong here.

0

In the example you gave, your partner is right. The two data helper builders are strategies. One is more specific than the other, but they are both interchangeable once initialized.

Basically if a client of the datahelperbuilder was to receive the systemdatahelperbuilder, nothing would break. Another option would have been to have the systemdatahelperbuilder be a static instance of the datahelperbuilder class.

protected by gnat Aug 26 '15 at 18:25

Thank you for your interest in this question. Because it has attracted low-quality or spam answers that had to be removed, posting an answer now requires 10 reputation on this site (the association bonus does not count).

Would you like to answer one of these unanswered questions instead?

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