5

I couldn't find a question that was not too specific to some case, so I'll try to make this very generic.

We need an extractor base class to a set of documents, for example. Each document has its specific properties, but they're ultimately documents. So we want to provide common extraction operations for all of them.

Even though they're all documents, as I said, they're somewhat different. Some may have some properties, but some may not.

Let's imagine that we have the Document base abstract class, and the FancyDocument and NotSoFancyDocument classes that inherit from it. The FancyDocument has a SectionA, the NotSoFancyDocument doesn't.

That said, what would you defend as the best way of implementing this? Here's the two options:

  • Empty virtual methods on the base class

Empty virtual methods on the base class would allow the programmer to only override the methods that make sense for the different types of documents. We would then have a default behavior on the abstract base class, which would be returning the default for the methods, like this:

public abstract class Document
{
    public virtual SectionA GetDocumentSectionA()
    {
        return default(SectionA);
    }
}

public class FancyDocument : Document
{
    public override SectionA GetDocumentSectionA()
    {
        // Specific implementation            
    }
}

public class NotSoFancyDocument : Document
{
    // Does not implement method GetDocumentSectionA because it doesn't have a SectionA
}
  • Concrete empty methods or concrete methods throwing a NotImplementedException

Since the NotSoFancyDocument does not have a SectionA, but the others do, we could either just return the default for the method in it, or we could throw a NotImplementedException. That would depend on how the program was written and some other things. We could come up with something like this:

//// Return the default value

public abstract class Document
{
    public abstract SectionA GetDocumentSectionA();
}

public class FancyDocument : Document
{
    public override SectionA GetDocumentSectionA()
    {
        // Specific implementation
    }
}

public class NotSoFancyDocument : Document
{
    public override SectionA GetDocumentSectionA()
    {
        return default(SectionA);
    }
}

OR

//// Throw an exception

public abstract class Document
{
    public abstract SectionA GetDocumentSectionA();
}

public class FancyDocument : Document
{
    public override SectionA GetDocumentSectionA()
    {
        // Specific implementation
    }
}

public class NotSoFancyDocument : Document
{
    public override SectionA GetDocumentSectionA()
    {
        throw new NotImplementedException("NotSoFancyDocument does not have a section A");
    }
}

Personally, I do think that the abstract method approach is better, since it means "Hey, I need you to be able to get a SectionA to be a document. I don't care how." while the virtual method means "Hey, I do have this SectionA here. If it's not good enough for you, feel free to change the way I get it.".

I do think the first one is a sign of object oriented programming smell.

What are your opinions on this?

4
  • I'd be inclined to make the methods that may or may not be implemented into individual interfaces, but even that has the same underlying problem. You want subclasses to be kind of like the base class sometimes, but not always. There's probably a bad abstraction involved.
    – Magus
    Mar 7, 2014 at 19:27
  • Why a SectionA? Why would the high level Document interface not specify Sections instead? When you say "all documents have a section A... oops, except this one type" that is an indication that either the one type should be separate, the document specifies too much of an interface, or maybe it is named incorrectly (not really a document).
    – user22815
    Mar 7, 2014 at 19:40
  • 1
    None of these suggestions are particularly good. If you have a finite, well-known set of concrete (non-polymorphic) types, the Visitor pattern is usually the best approach. However, be aware that the Visitor pattern violates the Open/Closed Principle... Mar 8, 2014 at 8:05
  • @MarkSeemann When you have a finite set of types, you're allowing extension of a different kind - it's easy to add new functions that operate on those types. It's the inverse of allowing an unlimited number of implementations, where it's easy to add new types but hard to add new functions (because it'd require updating an unbounded number of implementations.) I wouldn't say it violates Open/Closed Principle at all.
    – Doval
    Apr 9, 2014 at 12:21

3 Answers 3

4

In this case the base class should know nothing of SectionA. The derived class should implement the extra properties that that type needs.

For certain operations where another class needs to pull information out regardless of the type of document you will want that method on the base class ideally virtual with a basic implementation and allow derived classes to override it if needed (eg ToPlainText which would just output all the sections of a document would be on Document, FancyDocument can override the implementation to also output SectionA).

For instances where another class doesn't care about the type of document but does care if it has certain properties use interfaces. IDocument would have all the common sections and Document would implement it. IDocumentWithSectionA would inheerit IDocument and FancyDocument would implement that. This then lets you derive another NeitherFancyNorNotFancyDocument that has SectionA which can also implement IDocumentsWithSectionA.

Obviously You'd have more useful names than IDocumentWithSectionA but that depends on the usecase.

TL;DR use the abstract class for what should be common to all documents and some common functionality, use interfaces as the contract saying what a document has.

0

As stated in the comments, neither of these options is very good. That's probably the reason why you're asking this question as well.

The solution with empty virtual methods gives no information on what is supported by a specific document instance. Furthermore, it forces you to add all possible methods in the base class. This forces all clients to be updated as whel, when you decide that the base class needs additional methods.

The solution that throws exceptions is possibly even worse, since clients of the document classes will have to handle a possible exception for every method. That leads to dreadfull and unreliable code.


The problem that you are struggling with is that the document classes cannot predict the desired result. That is application logic, and that should not be implemented in the model layer. At least not in a way that leads to a reusable model layer.


I would suggest using interfaces to describe the sets of capabilities that you require. Fo instance, an IDocument interface, an IFancy interface and an INotSoFancy interface. These interface may or may not derive from IDocument or an other common interface, since you are talking about capabillities, not objects.

Using this interface based solution, client code can determine whether the given document instance supports a desired capabillity (by attempting to cast the document instance to the desired interface). The client code can than act accordingly.

0

I agree completely with Chao's answer and so I will not repeat that.

I think that the real problem here is that you need to ask yourself some more fundamental questions about the problem domain. Remember that all code is written to solve a problem.

Why are we writing a base Documentclass in the first place?

Why are we deriving from the Document class?

What do we hope to gain from deriving that we would not get by a mere interface?

In this case, it seems that there are two basic needs. There is the need to share code between document types and there is a need to get SectionA from FancyDocuments.

There is a number of ways to solve this problem and you have picked two of them. An alternative approach that you have not considered is to use composition instead of inheritance. In a lot of cases problems with inheritance are the result of it being the wrong tool for the job.

Consider the following as an alternative option:

sealed class Document 
{ 
    // some stuff 
}

class Fancy
{
    public Document Document { get; set; }
    public SectionA GetDocumentSectionA();
    // other fancy methods.
}

class NotFancy
{
    public Document Document { get; set; }
    // other non-fancy methods.
}

Now you can see that the delineation between the document types is much clearer. This disadvantage of this approach is that you cannot store both classes in the same strongly-typed Document container. If that is important for you, then you could consider using an interface or you could use inheritance instead.

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service and acknowledge that you have read and understand our privacy policy and code of conduct.

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