Is this actually good OO design?

Question

Context

I have Specification class with several properties including: StartDate, EndDate, DaysToComplete, DateToComplete.

A specification contains a list of required courses (i.e. a company requires employees to take training... perhaps a technical, diversity, or an ethics class). Based on a specification, courses will be mandated to employees.

Validation Concerns

StartDate and EndDate refer to the window of time in which a company can mandate a class to a user. DaysToComplete is how many days the employee has to compete training from the time it's mandated to them, whereas DateToComplete is the last day they have to complete the course.

• StartDate must always be specified
• DaysToComplete and DateToComplete are mutually exclusive... one and only one must be specified.
• If EndDate is not specified, then DaysToComplete must be used
• If StartDate = EndDate, then DateToComplete must be used

And there are other obvious validation rules such as the fact that you wouldn't have an EndDate greater than the DateToComplete since you wouldn't mandate a course after it already had to be completed.

The Problem

It seems as though there is a case here for having a base class and 4 derived classes with the following properties (i.e. this is how I was instructed to and did design the objects):

• SpecificationBase: StartDate
• PerpetualSpecification: DaysToComplete
• OneTimeSpecification*: DateToComplete
• FixedSpecification: EndDate, CompletionDate
• SlidingSpecification: EndDate, DaysToComplete

*When a user enters an EndDate that equals the StartDate they must use DateToComplete. In this case the EndDate is not actually saved though.

So we're using inheritance to do two things here: encapsulate validation rules and not expose properties that don't exist for particular scenarios.

As far as validation goes, from my perspective, the rules are always the same for all scenarios. Yes, certain properties don't apply in all scenarios but for the ones that do the validation logic really doesn't change. The argument, however, is that it is easier to comprehend the validation logic this way because putting it in all in the base class would be too complicated (writing a factory method to choose the right type based on user input and create it with reflection was, by comparison, considered not a big deal at all).

And as for hiding unused properties (or as my team lead insists: not exposing properties that don't exist), the argument started out as being an OO design principle that data and behavior should be coupled... until I pointed out that it would still be coupled in the base class without the derived classes (not to mention the data isn't decoupled in the database either).

I highly respect my team lead and from a purely philosophical/idealistic point of view this design seems great, but in practice it seems to create more problems than it solves:

• Had to create a factory method since we just get input from users without making them specify the type
• 3rd party tools that rely on reflection can't work with a list of different types with different properties so I have to create a proxy (or cheat and just put the data into an anonymous type and hope I'm never asked to provide a way to update the data from a grid control)
• Because consuming code only uses the factory method, it doesn't know what specific type it has or which properties are on it. This means if/then statements to test for what type I'm working with before trying to access a property that might not exist.

I'm sorry for the incredibly long essay, but I just wanted to provide ample context and background before asking my question: Is this 'really' good OO design? I feel like good OO design should make life easier... not harder. What am I missing here?

Edit: More Info

I realize now I left out a an important detail. My team lead is very, very big on requiring all data needed to initialize a proper type to be passed into its constructor (and not allowing a default constructor), and putting validation logic in the properties' set accessors. He feels this is the best way to prevent bad data from getting into the database. I don't particularly like this approach, however he's worked at some places where lack of good validation caused serious, serious problems.

So that's why I use a factory method... but the drawback of that is that type casting is required when the extended properties have to be worked with. Another drawback is that if I make the properties virtual then now, because of our validation approach, I have to call virtual properties from the constructor. Actually I ended up doing just that of necessity and practicality, but I'm didn't mention it in the beginning because doing so was a 'work around' to the actual design that I wanted to ask about.

Answer 1

Because consuming code only uses the factory method, it doesn't know what specific type it has or which properties are on it. This means if/then statements to test for what type I'm working with before trying to access a property that might not exist.

Now that's a code smell no doubt.

It's breaking encapsulation and not making proper use of polymorphism.

Whatever logic expressed by this switching on type, it should be contained in the class. Implementation details are not something your calling code should be concerned about.

Let's say that your calling code wants to know what's the actual last day (or end date) of the specification object. It's not interested how it's calculated or derived.

Based on your summary, I imagine your current implementation looks similar to this:

DateTime actualEndDate;
if (specification is PerpetualSpecification)
{
    // I ommit casting, don't use DateTime.Add etc. for clarity
    actualEndDate = specification.StartDate + specification.DaysToComplete;
}
else if (specification is OneTimeSpecification)
{
    actualEndDate = specification.DateToComplete;
}
else if (specification is FixedSpecification)
{
    actualEndDate = specification.EndDate != null 
        ? specification.EndDate 
        : specification.StartDate + specification.DaysToComplete;
}
// etc.

So it has to know a lot about how all these classes work internally. It's none of its business really. Plus, it introduces redundancy. It violates important design principles such as DRY as well as Single Source of Truth (if you need to compute actualEndDate in more than one place, these independent implementations are likely to become inconsistent over time).

If all the calling code wants to know is when comes the actual last day, implement it in your classes:

public abstract class SpecificationBase
{
    public abstract DateTime ActualLastDay
    {
        get;
    }    

    // ...
}

And then:

public class FixedSpecification : SpecificationBase
{
    public override DateTime ActualLastDay
    {
        get
        {
            return EndDate ?? StartDate + specification.DaysToComplete;
        }
        // writing a setter that deals with specific requirements 
        // of this particular class could also be possible actually
    }

    // ...
}

After that make your calling code just query specification for ActualLastDay property instead of if-elsing through all possible types.

Now it's no longer a problem that some properties only exist in some child classes.

You're creating a facade that will shield you from this.

(I can't rule out that I'm misunderstanding the question somehow - if my answer is off, some examples on your part would be appreciated)