RFC on “Factory” code [closed]

Question

This is the first code of this type that I've attempted, so I'm wondering if anybody would be willing to critique it. It's "sort of" (I think) a factory class. I want to keep it as simple as possible (and still work, and be at least as elegant as Phyllis Diller - it doesn't have to be Elizabeth Taylor).

The situation is that we may eventually support any number of printer types (printing labels from a Windows CE device), but want to keep the client code to something as simple as:

var beltPrinter = new BeltPrinter();
beltPrinter.PrintLabel(price, description, barcode);

Each print job will consist of 0..N preparatory commands sent to the printer ("hey, wake up!") followed by 1..N print commands, followed by 0..N shutdown commands.

Based on that, does the following make sense? If not, what would you advise that I change?

interface IBeltPrinter
    {
        private List<string> printerSetupCmds;
        private List<string> printLabelCmds;
        private List<string> printerTeardownCmds;

        void PrintLabel();
    }

class BeltPrinter : IBeltPrinter
{
    if (<ZebraQL220>) // don't know yet how I will determine which printer is in use - perhaps from reading a config file
    {
        return ZebraQL220Printer;
    }
    else if (<ONeal>) 
    {
        return ONealPrinter;
    }
}

class ZebraQL220Printer : IBeltPrinter
    {
        public void PrintLabel(string Price, string Desc, string Barcode)
        {
            var _price = Price;
            var _desc = Desc;
            var _barcode = Barcode;

            printerSetupCmds = new List<string>();
            printerSetupCmds.Add(''); // the format and commands, etc. will differ from other printers
            . . .
            printLabelCmds = new List<string>();
            printLabelCmds.Add('');
            . . .
            printerTeardownCmds = new List<string>();
            printerTeardownCmds.Add('');
            . . .

            foreach (string line in printerSetupCmds) {
    //send to ZebraQL220
            }
            foreach (string line in printLabelCmds) {
    //send to ZebraQL220
            }
            foreach (string line in printerTeardownCmds) {
    //send to ZebraQL220
            }
        }
    }

    class ONealPrinter : IBeltPrinter
    {
        public void PrintLabel(string Price, string Desc, string Barcode)
        {
            var _price = Price;
            var _desc = Desc;
            var _barcode = Barcode;

            printerSetupCmds = new List<string>();
            printerSetupCmds.Add(''); // the format and commands, etc. will differ from other printers
            . . .
            printLabelCmds = new List<string>();
            printLabelCmds.Add('');
            . . .
            printerTeardownCmds = new List<string>();
            printerTeardownCmds.Add('');
            . . .

            foreach (string line in printerSetupCmds) {
    //send to ONeal
            }
            foreach (string line in printLabelCmds) {
    //send to ONeal
            }
            foreach (string line in printerTeardownCmds) {
    //send to ONeal
            }
        }
    }

Answer 1

First off, I have to say this is a very clean and efficient design.

I'm hoping class IBeltPrinter BeltPrinter() is a typo, because I've never seen C# code like that.

Variable names for local variables (and parameters) should start with a lower case letter.

I would move the setup, labelCommands and tear down into their own methods. It will keep the PrintLabel method much cleaner

public void PrintLabel(string price, string desc, string barcode)
{
    ExecuteSetup(/*variables*/);
    ExecuteCommands(/*variables*/);
    ExecuteTeardown(/*variables*/);
}

Come to think of it, you could have a base class that has the PrintLabel method, and virtualExecuteSetup,ExecuteCommands, andExecuteTeardown` methods, that have generic printer commands. You could then extend that base class as required.

I would also consider putting the Factory definitions in an IDictionary. This will eliminate having a potentially huge switch statement.

private readonly static IDictionary<[whatever], IBeltPrinter> SupportedPrinters = new Dictionary<[whatever], IBeltPrinter>
    {
        {[ZebraQL220], new ZebraQL220Printer()},
        {[ONeal], new ONealPrinter()},
    }

Then your factory call would be

public IBeltPrinter CreatePrinter([whatever] printer)
{
    if (!SupportedPrinters.ContainsKey(printer))
        throw new PrinterNotSupportedException(printer)

    return SupportedPrinters[printer];
}

Answer 2

This is a textbook problem for polymorphism. Here's the fully working code:

void Main()
{
    var oNealPrinter = PrinterFactory.Create("ONeal");
    oNealPrinter.PrintLabel(new LabelDetail() { Price = "10", Description = "Hello", BarCode = "ABC" } );

    var zebraPrinter = PrinterFactory.Create("Zebra");
    zebraPrinter.PrintLabel(new LabelDetail() { Price = "10", Description = "Hello", BarCode = "ABC" } );   
}

public static class PrinterFactory
{
    //The following Create method you can put your logic of which printer is available.
    //Adjust the arguments and the code as per your need. 
    public static IBeltPrinter Create(string type)
    {
        if(string.IsNullOrWhiteSpace(type))
            throw new ArgumentNullException("type");

        if(type == "ONeal")
            return new ONealPrinter();

        if(type == "Zebra")
            return new ZebraQL220Printer();

        throw new InvalidOperationException("Can not create printer of type " + type);
    }
}

public interface IBeltPrinter
{
    void PrintLabel(LabelDetail labelDetail);
}

public class ONealPrinter : BeltPrinterBase
{
    protected override IEnumerable<string> SetupCommands 
    {
        get
        {
            yield return "ONeal Setup";
        }
    }

    protected override IEnumerable<string> LabelCommands 
    {
        get
        {
            yield return "ONeal Label";
        }
    }

    protected override IEnumerable<string> TearDownCommands 
    {
        get
        {
            yield return "ONeal Teardown";
        }
    }

}


public class ZebraQL220Printer : BeltPrinterBase
{
    protected override IEnumerable<string> SetupCommands 
    {
        get
        {
            yield return "Zebra Setup";
        }
    }

    protected override IEnumerable<string> LabelCommands 
    {
        get
        {
            yield return "Zebra Label";
        }
    }

    protected override IEnumerable<string> TearDownCommands 
    {
        get
        {
            yield return "Zebra Teardown";
        }
    }

}

public abstract class BeltPrinterBase : IBeltPrinter
{
    public LabelDetail LabelDetail { get; private set; }

    protected abstract IEnumerable<string> SetupCommands { get; }

    protected abstract IEnumerable<string> LabelCommands { get; }

    protected abstract IEnumerable<string> TearDownCommands { get; }

    IEnumerable<string> AllCommands 
    {
        get
        {
            return SetupCommands.Union(LabelCommands).Union(TearDownCommands);
        }       
    }

    public void PrintLabel(LabelDetail labelDetail)
    {
        if(labelDetail == null)
            throw new ArgumentNullException("labelDetail");

        LabelDetail = labelDetail;

        foreach (var command in AllCommands)
        {
            Console.WriteLine(command);
        }

        Console.WriteLine("Printed Label - " + labelDetail);
    }

}

//Creating a new LabelDetail class will capture the details as semantic
//unit and make future maintenance easier.
public class LabelDetail
{
    public string Price { get; set; }

    public string Description { get; set; }

    public string BarCode { get; set; }

    public override string ToString()
    {
        return "Price: " + Price + " Description: " + Description + " BarCode: " + BarCode;
    }
}

The output of the program is:

ONeal Setup
ONeal Label
ONeal Teardown
Printed Label - Price: 10 Description: Hello BarCode: ABC
Zebra Setup
Zebra Label
Zebra Teardown
Printed Label - Price: 10 Description: Hello BarCode: ABC