4
\$\begingroup\$

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.

\$\endgroup\$
  • \$\begingroup\$ Do BarCodeOperations and PageOperations both inherit from a base Operations class, or is Operations a namespace? \$\endgroup\$ – RubberDuck Sep 4 '14 at 15:20
  • \$\begingroup\$ What is the type of options.AddBarcodeVerb and options.AddBlankPageVerb? Are they constant or do they change according to something? \$\endgroup\$ – IEatBagels Sep 4 '14 at 15:46
  • \$\begingroup\$ Operations is just a namespace. options.StuffVerb are of type StuffOptions, and implement IProgramOptions, an interface describing parameters common to all verbs. Note that when I asked my question, it did not yet implement IProgramOptions. See more info here: github.com/gsscoder/commandline/wiki/Verb-Commands \$\endgroup\$ – thomasb Sep 5 '14 at 9:10
5
\$\begingroup\$

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

\$\endgroup\$
  • \$\begingroup\$ Thanks, it's a very interesting solution. I have updated my self-answer to provide more examples. With your implementation, how would you use a variable that is declared outside of your DoRealStuff method? I sometimes have to create images beforehand, or increment/decrement a variable, that I will use inside my actual workload. Is it possible with your solution ? \$\endgroup\$ – thomasb Sep 5 '14 at 9:46
  • \$\begingroup\$ Yes, look at my EDITed part. I cannot write more now, but let me know if you are able to go from there or do you need full examples. I migth be able to provide them later in the evening... \$\endgroup\$ – SebGruch Sep 5 '14 at 15:43
  • \$\begingroup\$ Hm no I'm sorry but I don't really understand your edit. Don't worry I'm not in a hurry ;) \$\endgroup\$ – thomasb Sep 5 '14 at 16:32
  • \$\begingroup\$ @thomasbtv Hi, I'd added some xtra explanation on alternatives. Sorry for late answer. \$\endgroup\$ – SebGruch Sep 15 '14 at 11:57
  • \$\begingroup\$ Thanks! I understand better now. I think my "special" cases are a bit too much specific each time, so I'll just keep my delegates-based solution for now. But thank you for the extensive answer and explanations :) \$\endgroup\$ – thomasb Sep 15 '14 at 14:15
4
\$\begingroup\$

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.

\$\endgroup\$
  • \$\begingroup\$ Seems quite elegant to me and has much less syntax bloat compared to a class oriented solution. \$\endgroup\$ – ChaosPandion Sep 4 '14 at 15:36
3
\$\begingroup\$

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!

\$\endgroup\$
  • \$\begingroup\$ Why do you think inheritance is a better solution than delegates? \$\endgroup\$ – ChaosPandion Sep 4 '14 at 17:44
  • \$\begingroup\$ In my opinion, delegates are harder to understand than inheritance, plus you sort of loose the sense of your object using delegates. Using inheritance, you have consistent objects that have a behavior. I'm not able to explain it in better details in english unfortunatly.. \$\endgroup\$ – IEatBagels Sep 4 '14 at 19:01
  • 1
    \$\begingroup\$ I can understand your position. My position was once the same. Over time I learned that objects are not always necessary for composing behavior. Once your brain becomes used to the idea of composing functions rather than objects it can be a very useful tool. \$\endgroup\$ – ChaosPandion Sep 4 '14 at 19:34
  • \$\begingroup\$ Thanks for your approach, even though I find the answer from @SebGruch more complete in this regard. Actually the foreach loop is in 10 operations out of 13 (2 of which work very differently anyway), so for now I think I'll include it. I might create another method without this loop if the need arises. I'll update my self-answer to provide more details and examples. \$\endgroup\$ – thomasb Sep 5 '14 at 9:38
  • \$\begingroup\$ @ChaosPandion, could you post an answer that would explain your point, I have difficulties understanding what you mean. \$\endgroup\$ – IEatBagels Sep 5 '14 at 12:10

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service, privacy policy and cookie policy

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