0

The code below describes my design issue in detail. I have a task table in a database that can have different types of recurrence patterns. The task table has columns for each of the possible recurrence pattern fields. However, I want the Task object to create the proper pattern based on which pattern is in the db. The code below will do accomplish this, but then the problem is the calling code will always have to check the type of Recurrence being returned before taking any action.

e.g.

var t = new Task();
var pattern = t.Recurrance;

The calling code has no idea what type of recurrance is being created?

What is a better way to model this?

class Task
{
    private int recurrenceType = 0; //pulled from the db
    public Task()
    {
        //determine recurrence type from database 
        switch (recurrenceType)
        {
            case 0:
                Recurrance = new RecurrenceDaily();
                break;
            case 1:
                Recurrance = new RecurrenceMonthly();
                break;
            case 2:
                Recurrance = new RecurrenceWeekly();
                break;
        }
    }
    public RecurrenceBase Recurrance { get; set;}
}

abstract class RecurrenceBase
{
    public int Frequency { get; set; }
}

class RecurrenceDaily : RecurrenceBase
{
    public bool Weekends { get; set; }
}

class RecurrenceWeekly : RecurrenceBase
{
    public DaysOfWeekFlagsEnum DaysOfWeek { get; set; }
}

class RecurrenceMonthly : RecurrenceBase
{
    public byte DayOfMonth { get; set; }
    public WeekEnum Week { get; set; }
    public DayOfWeekEnum DayOfWeek { get; set; }
}
Pete Maroun
  • 2,045
  • 2
  • 18
  • 27

4 Answers4

0

Well, you have a number of problems in this code. For starters, your Recurrance will always be RecurranceDaily because you create the Recurrance in the constructor, with no way to set the RecurranceType prior to creating the Recurrance. The object has to be created before any data in the object can be set. Unless you set that data in the constructor itself, then you will always have the default state (which is 0 for ints, plus you initialize it to 0).

Second, you have no setter for the recurranceType, and since the value is private, there is no way to set it after construction.

Third, Your Recurrance member is a property that has only a getter, yet in the constructor you're setting it's value. This won't compile either, because you have no set.

Erik Funkenbusch
  • 92,674
  • 28
  • 195
  • 291
0

I'm not sure you've given us enough information to make an intelligent suggestion about the design direction, but you might want to consider having different types of tasks based on the recurrence type, e.g., DailyTask, WeeklyTask, MonthlyTask.

You could then place the appropriate logic in the Task subclass. For example.

var task = db.GetTaskById( 15 );
task.Schedule( startDate );

Your database code would get the task by id, figure out that it's a WeeklyTask, for example, and set up the proper recurrence within it. It would then have a Schedule method that makes sense for a weekly task starting on startDate.

public interface ITask
{
   public void Schedule( DateTime startDate );
}

public abstract class TaskBase : ITask
{
   // common methods...
   public abstract void Schedule( DateTime startDate );
}

public class WeeklyTask : TaskBase
{
   public override void Schedule( DateTime startDate )
   {
       var startWeek = GetStartOfWeek( startDate );
       var firstDate = startWeek.AddDays( DaysOfWeek[0] );
       ...
   }
}
vrajs5
  • 4,066
  • 1
  • 27
  • 44
tvanfosson
  • 524,688
  • 99
  • 697
  • 795
  • that could also work.. returning back a differnt type of task (instead of a different recurrence pattern). However, the question still remains... It is just moved to the `var task = db.GetTaskById( 15 );` call as this call will have to return the base class. The only way the caller would know which type of task is returned is by performing a type check on the returned object. – Pete Maroun Apr 22 '11 at 21:50
  • @pmaroun - but the other part is to move what you are doing with it into the specific task subclass. That is, each task contains the code to operate on it. It exposes its operations via the interface. In this example, I've used the single `Schedule` method, but any other operations to be done on tasks would be handled similarly. This way the calling code need only know about the interface contract and not have different code for each task type. This is a fundamental OO principle, encapsulation: each class should contain the both the data **and** the code to operate on that data. – tvanfosson Apr 22 '11 at 21:56
0

Obviously there is more to your scenario than meets the eye here, but I would look to this answer from SO About Inversion of Control And Dependency Injection. Then go on to The Martin Fowler Article. Using IoC and DI you will abstract your RecurrenceBase to something more like an IRecur interface with a "DoSomething" event etc. to create commonalities. Perhaps a "RecurrenceEventArgs" class that encapsulates the scheduling.

I hope this gets you pointed in the right direction.

Community
  • 1
  • 1
Cos Callis
  • 5,051
  • 3
  • 30
  • 57
0

You are creating an inheritance hierarchy - an OO construct - but the classes you show are decidedly not object oriented, since they contain no behavior and only expose data.

This is an improper use of inheritance and at least part of what's causing you pain.

When you inherit, you are stating that sub-classes can be used with the same contract as the parent class. With the classes you are showing and the way you are using them, this is simply not true.

quentin-starin
  • 26,121
  • 7
  • 68
  • 86