Can someone please critique my Factory Pattern?

Question

I have recently been learning more about design patterns and thought I'd take a crack at it. I'm not sure if this is the correct way to use the factory pattern and was wondering if someone could provide me with feedback?

I basically have a set of calendar items that are similar. Meals, workouts and measurements. They all have a date and a name and can print details. but other than that they have a few items they each need differently.

    public enum AppointmentType
{
    Workout,
    Meal,
    Measurement
}

public abstract class Appointment
{
    public string Name { get; set; }
    public DateTime DateStarted { get; set; }

    public virtual string PrintDetails()
    {
        return string.Format("Type: {0}\nName: {1}\nDate: {2}",this.GetType().ToString(), Name, DateStarted.ToShortDateString());
    }
}

public class Workout : Appointment
{
    public List<string> Exercises { get; set; }

    public Workout()
    {
        Exercises = new List<string>();
    }

    public override string PrintDetails()
    {
        string startInfo = base.PrintDetails();
        string addedInfo = "\nToday I will be doing the following:\n";

        foreach (string exercise in Exercises)
        {
            addedInfo += string.Format("{0}\n", exercise);
        }

        return startInfo + addedInfo;
    }
}

public class Meal : Appointment
{
    public List<string> FoodItems { get; set; }

    public Meal()
    {
        FoodItems = new List<string>();
    }

    public override string PrintDetails()
    {
        string startInfo = base.PrintDetails();
        string addedInfo = "\nToday I will be eating the following:\n";

        foreach (string foodItem in FoodItems)
        {
            addedInfo += string.Format("{0}\n", foodItem);
        }

        return startInfo + addedInfo;
    }
}

public class Measurement : Appointment
{
    public string Height { get; set; }
    public string Weight { get; set; }

    public override string PrintDetails()
    {
        string startInfo = base.PrintDetails();
        string addedInfo = string.Format("\nI am {0} feet tall and I weight {1} pounds!\n", Height, Weight);
        return startInfo + addedInfo;
    }
}

public interface IAppointmentFactory
{
    Appointment CreateAppointment(AppointmentType appointmentType);
}

public class AppointmentFactory : IAppointmentFactory
{
    public Appointment CreateAppointment(AppointmentType appointmentType)
    {
        switch (appointmentType)
        {
            case AppointmentType.Workout:
                return new Workout();
            case AppointmentType.Meal:
                return new Meal();
            case AppointmentType.Measurement:
                return new Measurement();
            default:
                return new Workout();
        }
    }
}

and here is a program using the library:

  class Program
{
    public static List<Appointment> myAppointments;
    public static AppointmentFactory factory;
    public static Appointment myAppointment;

    static void Main(string[] args)
    {
        myAppointments = new List<Appointment>();
        factory = new AppointmentFactory();
        StartLoop();
    }

    private static void StartLoop()
    {
        Console.WriteLine(string.Format("\nWelcome to Appointment App: You have {0} appointment(s)!", myAppointments.Count()));
        Console.WriteLine("0 = Exit");
        Console.WriteLine("1 = New Workout");
        Console.WriteLine("2 = New Meal");
        Console.WriteLine("3 = New Measurement");
        Console.WriteLine("P = Print Schedule\n");

        switch (Console.ReadLine().ToUpper())
        {
            case "0":
                return;
            case "1":
                CreateNewAppointment(AppointmentType.Workout);
                AddExercises();
                break;
            case "2":
                CreateNewAppointment(AppointmentType.Meal);
                AddFoodItems();
                break;
            case "3":
                CreateNewAppointment(AppointmentType.Measurement);
                GetMeasurements();
                break;
            case "P":
                PrintSchedule();
                break;
            default:
                return;
        }

        StartLoop();
    }

    private static void GetMeasurements()
    {
        Console.WriteLine("How tall are you?");
        ((Measurement)myAppointment).Height = Console.ReadLine();
        Console.WriteLine("What is your weight?");
        ((Measurement)myAppointment).Weight = Console.ReadLine();
    }

    private static void AddFoodItems()
    {
        Console.WriteLine("How many food items do you want to add?");
        string exerciseCount = Console.ReadLine();

        for (int i = 0; i < Convert.ToInt32(exerciseCount); i++)
        {
            Console.WriteLine(string.Format("Food {0}:", (i + 1)));
            ((Meal)myAppointment).FoodItems.Add(Console.ReadLine());
        }
    }

    private static void AddExercises()
    {
        Console.WriteLine("How many exercises do you want to add?");
        string exerciseCount = Console.ReadLine();

        for (int i = 0; i < Convert.ToInt32(exerciseCount); i++)
        {
            Console.WriteLine(string.Format("Exercise {0}:", (i + 1)));
            ((Workout)myAppointment).Exercises.Add(Console.ReadLine());
        }
    }

    private static void PrintSchedule()
    {
        foreach (Appointment appointment in myAppointments)
        {
            Console.WriteLine(appointment.PrintDetails());
        }
    }

    public static void CreateNewAppointment(AppointmentType appointmentType)
    {
        myAppointment = factory.CreateAppointment(appointmentType);

        Console.WriteLine("Name:");
        myAppointment.Name = Console.ReadLine();
        Console.WriteLine("Start Date:");
        myAppointment.Name = Console.ReadLine();

        myAppointments.Add(myAppointment);
    }
}

Thanks!!

-ajax

Answer 1

Here's what I would change it to:

public enum AppointmentType
{
    Workout,
    Meal,
    Measurement
}

public abstract class Appointment
{
    public string Name { get; set; }
    public DateTime DateStarted { get; set; }

    public abstract void PrintDetails();
    public abstract void RequestInputFromUser();
}

public class Workout : Appointment
{
    private List<string> Exercises { get; set; }

    public Workout()
    {
        Exercises = new List<string>();
    }

    public override void PrintDetails()
    {
        string startInfo = base.PrintDetails();
        string addedInfo = "\nToday I will be doing the following:\n";

        foreach (string exercise in Exercises)
        {
            addedInfo += string.Format("{0}\n", exercise);
        }

        Console.WriteLine(StartInfo + addedInfo);
    }

    public override void RequestInputFromUser()
    {
        Console.WriteLine("How many exercises do you want to add?");
        string exerciseCount = Console.ReadLine();

        for (int i = 0; i < Convert.ToInt32(exerciseCount); i++)
        {
            Console.WriteLine(string.Format("Exercise {0}:", (i + 1)));
            Exercises.Add(Console.ReadLine());
        }
    }
}

public class Meal : Appointment
{
    private List<string> FoodItems { get; set; }

    public Meal()
    {
        FoodItems = new List<string>();
    }

    public override void PrintDetails()
    {
        string startInfo = base.PrintDetails();
        string addedInfo = "\nToday I will be eating the following:\n";

        foreach (string foodItem in FoodItems)
        {
            addedInfo += string.Format("{0}\n", foodItem);
        }

        Console.WriteLine(startInfo + addedInfo);
    }

    public override void RequestInputFromUser()
    {
        Console.WriteLine("How many food items do you want to add?");
        string exerciseCount = Console.ReadLine();

        for (int i = 0; i < Convert.ToInt32(exerciseCount); i++)
        {
            Console.WriteLine(string.Format("Food {0}:", (i + 1)));
            FoodItems.Add(Console.ReadLine());
        }
    }
}

public class Measurement : Appointment
{
    private string Height { get; set; }
    private string Weight { get; set; }

    public override void PrintDetails()
    {
        string startInfo = base.PrintDetails();
        string addedInfo = string.Format("\nI am {0} feet tall and I weight {1} pounds!\n", Height, Weight);
        Console.WriteLine(startInfo + addedInfo);
    }

    public override void RequestInputFromUser()
    {
        Console.WriteLine("How tall are you?");
        Height = Console.ReadLine();
        Console.WriteLine("What is your weight?");
        Weight = Console.ReadLine();
    }
}

public interface IAppointmentFactory
{
    Appointment CreateAppointment(AppointmentType appointmentType);
}

public class AppointmentFactory : IAppointmentFactory
{
    public Appointment CreateAppointment(AppointmentType appointmentType)
    {
        Appointment apt;

        switch (appointmentType)
        {
            case AppointmentType.Workout:
                apt = new Workout();
            case AppointmentType.Meal:
                apt = new Meal();
            case AppointmentType.Measurement:
                apt = new Measurement();
            default:
                apt = new Workout();
        }

        Console.WriteLine("Name:");
        apt.Name = Console.ReadLine();
        Console.WriteLine("Start Date:");
        apt.Name = Console.ReadLine();    // Logic error on this line.
                                          // Change it to do what you mean.

        apt.RequestInputFromUser();

        return apt;
    }
}

with the code using it as follows:

class Program
{
    public static List<Appointment> myAppointments;
    public static AppointmentFactory factory;
    public static Appointment myAppointment;

    static void Main(string[] args)
    {
        myAppointments = new List<Appointment>();
        factory = new AppointmentFactory();
        StartLoop(factory);
    }

    private static void StartLoop(IAppointmentFactory factory)
    {
        bool exit = false;

        while(!exit)
        {

            Console.WriteLine(string.Format("\nWelcome to Appointment App: You have {0} appointment(s)!", myAppointments.Count()));
            Console.WriteLine("0 = Exit");
            Console.WriteLine("1 = New Workout");
            Console.WriteLine("2 = New Meal");
            Console.WriteLine("3 = New Measurement");
            Console.WriteLine("P = Print Schedule\n");

            switch (Console.ReadLine().ToUpper())
            {
                case "0":
                    exit = true;
                    break;
                case "1":
                    myAppointments.Add( factory.CreateAppointment( AppointmentType.Workout ) );
                    break;
                case "2":
                    myAppointments.Add( factory.CreateAppointment( AppointmentType.Meal ) );
                    break;
                case "3":
                    myAppointments.Add( factory.CreateAppointment( AppointmentType.Measurement ) );
                    break;
                case "P":
                    PrintSchedule();
                    break;
                default:
                    exit = true;
            }

        }

    }

    private static void PrintSchedule()
    {
        foreach (Appointment appointment in myAppointments)
        {
            appointment.PrintDetails();
        }
    }

}

A couple things to notice:

• All properties in the lower classes are private (a good thing)
• There is no type casting (another good thing)
• It is easy to add another Appointment type (another good thing)

There are still things that can change. The base class having public properties is still a bad thing. But this is a large improvement.

I also removed the recursive call to StartLoop(). You don't want to grow the stack when all you need is a loop.

EDIT: Made some more changes to the Factory class.

In response to comments:

• To remove the dependency on the Console, create an interface that represents what you want to do with IO. Create classes that implement this interface and send an instance to the Appointment class. From there, have everything use the instance of the IO engine. To change the platform, change the IO-implementing class that is sent to the Appointment class
• Public properties are bad for the same reason that public variables are bad. A class should hide its implementation, and public properties expose the implementation. For more detail, I suggest Clean Code. It describes it much better than I can.

Overall, not bad :)

Answer 2

Well, for starters - you code follows the pattern nicely.

Your particular implementation suffers from a few issues (nothing that cannot be worked around though):

1. Using an enum for the parameter violates the OCP principle. If you add or worse yet, remove a value to/from the enum, clients will have to recompile to use the correct (new) enum. For the textbook answer - you could use a string to represent the type you want to create and then throw exceptions/handle it in some other way when a non-supported type is requested.

2. Adding a new Factory-implementation will be forced to only support the same return types as the original factory can handle - unless you can re-compile (violating OCP again). Depending on your scenario - this might be okay.

But other than that - in my oppinion it is a viable implementation.

Answer 3

As I understand it, the abstract factory pattern is used when you need to create different sets of the same kinds of objects. So, for instance, you might have a factory which vends

Supersize-Workout
Supersize-Meal
Supersize-Measurement

and another which vends

Mini-Workout
Mini-Meal
Mini-Measurement

These factory classes could then be fed to a system which needs to create workouts, meals and measurements but doesn't care if they are supersize or mini, as long as they're all the same.

In the absence of a second factory, I find your abstract factory a bit odd.