Command-line program for applying several operations on a PDF file

Question

I am building a command-line .exe that can apply several operations on a PDF file (add text, images, resize, crop, etc).

Currently, my Program.cs looks a bit like this (it uses CommandLineParser):

switch (invokedVerb)
{
    case "barcode":
        Operations.BarcodeOperations.AddBarCode(options.AddBarcodeVerb);
        break;

    case "addblankpage":
        Operations.PageOperations.AddBlankPage(options.AddBlankPageVerb);
        break;
}

As you can see, I have split the operations into several XXXOperations classes, each of them having very similar instructions:

public static void AddStuff(StuffOptions options)
{
    Logging.Log("here is a bunch of logging");

    // here sometimes there is some action-specific code but not often

    using (DocWrapper doc = new DocWrapper(options.File)) // this is in all actions
    {
        foreach (int page in doc.GetPagesToModify(options.Pages)) // this is in most actions
        {
            // call some stuff on the doc instance
        }

        doc.Save(options.OutputFile); // this is in all actions
    }
}

So, all actions create a new instance of DocWrapper, most of them loop on its pages (but I could modify the actions so that all of them do), and all of them save, but each of them do a different set of actions inside it.

How could I refactor this code so that the DocWrapper instantiation, the pages loop and the save are in a single place, but I can specify custom code inside the loop?

I'm thinking of using delegates to define my actions, but I have no idea where to start, since I'm not very familiar with them. I'm also considering inheriting from an abstract class that implements the loop and abstracts a DoWork method, called inside the loop and implemented in the XXXOperations class, but since I don't know delegates, I'm not sure which is more adapted to the situation.

Answer 1

Yep. I suppose your idea is generaly fine. This is being called Command Pattern. You might like to read more about it on Google. Used together with other well known pattern Strategy they both might be used here. Plus another one - Template Method. Let me just suggest some basic approach:

public abstract class BaseCommand
{
    protected BaseCommand()
    {

    }

    public void Process(StuffOptions options)
    {
        Logging.Log("here is a bunch of logging");

        // here sometimes there is some action-specific code but not often
        ExtraStuff();

        using (DocWrapper doc = new DocWrapper(options.File)) // this is in all actions
        {
            foreach (int page in doc.GetPagesToModify(options.Pages)) // this is in most actions
            {
                // call some stuff on the doc instance
                DoRealStuff(doc);
            }

            doc.Save(options.OutputFile); // this is in all actions
        }
    }

    protected abstract void DoRealStuff(DocWrapper doc);

    protected virtual void ExtraStuff()
    {

    }
}

So, now you can go and implement your commands. You have to ALWAYS implement abstract method DoRealStuff() but only override ExtraStuff() when needed.

class BaseCommandA : BaseCommand
{
    protected override void DoRealStuff(DocWrapper doc)
    {
        // Doing my stuff here!
    }
}

class BaseCommandB : BaseCommand
{
    protected override void ExtraStuff()
    {
        base.ExtraStuff();

        // I have to set some extra paramaetrs here...
    }

    protected override void DoRealStuff(DocWrapper doc)
    {
        // Doing my other stuff here!
    }
}

Then there is matter of implementing last part - strategy. You can keep your static switch-get approach if you like, bu t better is to hide your classes details and return commands only by their base type:

public static class CommandFactory
{
    private static BaseCommandA commandAInstance = new BaseCommandA();
    private static BaseCommandB commandBInstance = new BaseCommandB();

    public static BaseCommand GetCommand(invokedVerb)
    {
        switch (invokedVerb)
        {
            case "barcode": return commandAInstance;
            case "addblankpage": return commandBInstance;

            // ...
        }
    }
}

You might like to go into some lazy initialization inside if you like...

And then - grande finale - using all this stuff:

    public void Processing(cmd)
    {
        CommandFactory.GetCommand(cmd.Verb).Process(cmd.Options);
        // ...
    }

Ofcourse all types, and arguments are being simplified. Good luck and have nice coding.

EDIT: And yes - there ofcourse are some precautions - when commands are done like that (static) all processing shall be stateless (no properties or fields inside command classes). If this is required - for example to pass some data from DoExtraStuff() - you just remove static fields and create new instance of command every time the GetCommand() is being called. Good thing is - the main Processing function remains the same - power of encapsulation. Delegate approach might also be fine, but I think my classic solution is much easier to expand, extend and maintain.

EDIT 2: I was asked by OP to explain more situation where there is some data to pass between preprocessing function and main function. There are at least two possible solutions. First is to implement factory other way, so this is no longer using static instances of commands but creates a new one for each call:

public static class CommandFactory2
{
    public static BaseCommand GetCommand(string invokedVerb)
    {
        switch (invokedVerb)
        {
            case "barcode": return new CommandA();
            case "addblankpage": return new CommandB();

            // ...
        }

        return null;
    }
}

The only remaining thing is to add some fields / properties to BaseCommand or specific Command and just share values between methods of the class, like:

class CommandC : BaseCommand
{
    private string _extraValue = null;

    protected override void ExtraStuff()
    {
        base.ExtraStuff();
        _extraValue = ProcessExtraValue();

        // I have to set some extra parameters here...
    }

    protected override void DoRealStuff(DocWrapper doc)
    {
        // Doing my other stuff here!
        switch (_extraValue)
        {
           // ...
        }
    }

    private string ProcessExtraValue()
    {
        //...
        return "";
    }
}

The second approach would be suitable if you always produce same intermediate data, so then you do not alter Factory but rather BaseCommand interface and children, like:

public abstract class BaseCommand
{
    protected BaseCommand()
    {

    }

    public void Process(StuffOptions options)
    {
        Logging.Log("here is a bunch of logging");

        // here sometimes there is some action-specific code but not often
        ExtraData data = new ExtraData();
        ExtraStuff(data);

        using (DocWrapper doc = new DocWrapper(options.File)) // this is in all actions
        {
            foreach (int page in doc.GetPagesToModify(options.Pages)) // this is in most actions
            {
                // call some stuff on the doc instance
                DoRealStuff(doc, data);
            }

            doc.Save(options.OutputFile); // this is in all actions
        }
    }

    protected abstract void DoRealStuff(DocWrapper doc, ExtraData data);

    protected virtual void ExtraStuff(ExtraData data)
    {

    }
}

And then you just fill this ExtraData class with whathever proeprties you wan't to share.

I would personally prefer first one, as it is more elastic and introduces extra stuff only when this is required (ie. only inside these commands that need this). However drawback is if you are planning on intense processing - you will be creating new instance of command class every time you are going to use it...

Eventually, this can be solved on factory level - implement each command creation the best way that suits it better - even create deeper command inheritance to distinguish different behavior. But now I went too far (and broke SOLID a bit), so let allow me to finish at this point :P

Answer 2

Here is what I have done so far:

I have created a Worker class with my redundant code:

public static void DoWorkOnPages(IProgramOptions options, Action<DocWrapper, int> actions)
{
    using (DocWrapper doc = new DocWrapper(options.File))
    {
        foreach (int page in doc.GetPagesToModify(options.Pages).OrderBy(p => p))
        {
            actions(doc, page);
        }

        doc.Save(options.OutputFile);
    }
}

And in each XXXOperations class my methods calls it like this:

public static void AddStuff(StuffOptions options)
{
    Logging.Log("Here is a magnificient function");

    // example of code that is different from other operations
    int someVar = 0;
    using (Image barcode = CreateBarcodeImage(someParameters))
    {
        Worker.DoWorkOnPages(options, (doc, page) =>
            {
                doc.SetPage(page);
                doc.AddImage(barcode);
                doc.DoSomething(options.SomeProperty, someVar);
                someVar++;
            });
    }
}

Obviously, for in the operations that have a very different behavior, I had to duplicate some code, but I guess it can't be helped.

If you come up with a more elegant, more simple, more powerful solution or just a different one, I'll gladly upvote it.

Answer 3

I think you should use inheritance over delegates, using a template design pattern this way:

public abstract class Operation<T>
{
    public void Add(T options)
    {
        Logging.Log("here is a bunch of logging");

        ActionSpecificCodeNotOftenUsed();

        using (DocWrapper doc = new DocWrapper(options.File))
        {
            DoSomethingWithOperation(doc);

            doc.Save(options.OutputFile);
        }

    protected abstract void DoSomethingWithOperation(DocWrapper doc);
    protected abstract void ActionSpecificCodeNotOftenUsed();
}

Excuse the method names, your comments aren't very self-explanatory so I did with what I had! Feel free to correct me.

Then you can implement your class

public class BarcodeOperations : Operation</*type of options.AddBarcodeVerb*/>
{
    protected override void DoSomethingWithOperation(DocWrapper doc)
    {
        //Doing whatever you want
    }

    protected override void ActionSpecificCodeNotOftenUsed()
    {
        //Do something, or nothing!
    }
}

In the end, you can use this class this way :

new BarCodeOperations().Add(options.AddBarcodeVerb);

I didn't do anything about the foreach since it shouldn't worry you too much to repeat the loop in some operations, adding reusability for a loop seams a little overkill and would probably make the code harder to maintain.

I'll ask you some questions in the comment of you post, if you answer them I might have some other things to review, but for now that is it!