0
\$\begingroup\$

I feel like there is a cleaner/simpler way of doing the following. Is there an existing software pattern that I should be using?

I have an Assembler class that I use to assemble/process content rules MS-Word documents.

There are many different types of Controls that are processed:

  • ClearCellIf
  • DeleteColumnIf
  • DeleteLineIf
  • DeleteRowIf
  • RepeatingControl

The controls fall into two categories:

  1. Generic controls (used over and over)
  2. Custom controls (only used in a specific solution)

Each control extends a base PrecedentControl class and has a Test and a Process method:

class DeleteRowIf : PrecedentControl
{
    public DeleteRowIf(Wd.ContentControl control) : base(control) { }

    public static bool Test(Wd.ContentControl control, PrecedentInstruction Instruction)
    {
        return Instruction != null && Instruction.Command == "DeleteRowIf";
    }

    public override void Process<T>(T data, ref bool Cancel)
    {
        var result = PrecedentExpression1.Resolve(control.GetPrecedentInstruction().Expression, data);
        if (result)
            control.DeleteRow();
    }
}

The bit that I hate, and would like to do better, is the logic that determines which type of control is in use:

public static PrecedentControl AsPrecedentControl(this Wd.ContentControl control)
{
    var instruction = control.GetPrecedentInstruction();
    if (FormatVariable.Test(control, instruction))
        return new FormatVariable(control);
    else if (Optional.Test(control, instruction))
        return new Optional(control);
    else if (DeleteControlIf.Test(control, instruction))
        return new DeleteControlIf(control);
    else if (DeleteControlIf_AndPreviousSpace.Test(control, instruction))
        return new DeleteControlIf_AndPreviousSpace(control);
    else if (DeleteControlIf_AndSurroundingPunctuation.Test(control, instruction))
        return new DeleteControlIf_AndSurroundingPunctuation(control);
    else if (DeleteLineIf.Test(control, instruction))
        return new DeleteLineIf(control);
    else if (DeleteLineIf_OrRemoveControl.Test(control, instruction))
        return new DeleteLineIf_OrRemoveControl(control);
    else if (DeleteLineIf_AndRepositionBulletEnding.Test(control, instruction))
        return new DeleteLineIf_AndRepositionBulletEnding(control);
    else if (DeleteRowIf.Test(control, instruction))
        return new DeleteRowIf(control);
    else if (DeleteRowIf_OrRemoveControl.Test(control, instruction))
        return new DeleteRowIf_OrRemoveControl(control);
    else if (DeleteColumnIf.Test(control, instruction))
        return new DeleteColumnIf(control);
    else if (ClearCellIf.Test(control, instruction))
        return new ClearCellIf(control);
    else if (RepeatingControl.Test(control, instruction))
        return new RepeatingControl(control);
    else if (RepeatingControl_RemoveLastAnd.Test(control, instruction))
        return new RepeatingControl_RemoveLastAnd(control);
    else if (RepeatingControl_SecondLastHasAnd.Test(control, instruction))
        return new RepeatingControl_SecondLastHasAnd(control);
    else if (RunMacro.Test(control, instruction))
        return new RunMacro(control);
    else if (ContactDelivery.Test(control, instruction))
        return new ContactDelivery(control);
    else if (ContactDelivery_DeleteControlAndPrecedingComma.Test(control, instruction))
        return new ContactDelivery_DeleteControlAndPrecedingComma(control);
    else if (ContactDelivery_MultilineAddress.Test(control, instruction))
        return new ContactDelivery_MultilineAddress(control);
    else
        return null;
}
\$\endgroup\$
0

1 Answer 1

2
\$\begingroup\$

If all the Test methods look like in the provided sample implementation then I see several approaches to make your factory method cleaner:

  1. Use a switch:

    public static class Instructions
    {
        public static const string DeleteIf = "DeleteIf";
        public static const string Optional = "Optional";
        ...
    }
    
    public static PrecedentControl AsPrecedentControl(this Wd.ContentControl control)
    {
        switch (instruction.Command)
        {
            case Instructions.DeleteIf: return new DeleteIf(control);
            case Instructions.Optional: return new Optional(control);
            ...
            default:
                throw new InvalidInstructionException(instruction);
        }
    }
    
  2. Use a data driven approach with a Dictionary:

    Dictionary<string, Type> InstructionMap = new Dictionary<string, Type> 
    {
        { Instructions.DeleteIf, typeof(DeleteIf) },
        { Instructions.Optional, typeof(Optional) },
        ...
    }
    

    And your factory method being:

    public static PrecedentControl AsPrecedentControl(this Wd.ContentControl control)
    {
        Type instructionType = null;
        if (InstructionMap.TryGet(instruction.Command, out instructionType))
        {
            return Activator.CreateInstanceOf(instructionType, control);
        }
        throw new InvalidInstructionException(instruction);
    }
    

    If by convention the instruction commands are all named the exact same as the corresponding classes dealing with them you can build the dictionary dynamically once via reflection by finding all the types deriving from PrecedentControl. Something along these lines might work:

    typeof(PrecedentControl).Assembly
                            .GetTypes()
                            .Where(t => !t.IsAbstract && typeof(PrecedentControl).IsAssignableFrom(t))
                            .ToDictionary(t => t.Name, t => t);
    
\$\endgroup\$
3
  • \$\begingroup\$ Is there a way that doesn't require the if/switch? I often create a new Control class and forget to add it to the if/switch. \$\endgroup\$ Commented Feb 11, 2016 at 8:58
  • \$\begingroup\$ @MattFitzmaurice: That's option 2 with loading the types from the assembly by convention. You just need to add your control class and be done with it. \$\endgroup\$
    – ChrisWue
    Commented Feb 11, 2016 at 19:50
  • \$\begingroup\$ Will give it a go. Cheers \$\endgroup\$ Commented Feb 12, 2016 at 3:40

Your Answer

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

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