Take the 2-minute tour ×
Code Review Stack Exchange is a question and answer site for peer programmer code reviews. It's 100% free, no registration required.

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.

share|improve this question
    
Do BarCodeOperations and PageOperations both inherit from a base Operations class, or is Operations a namespace? –  RubberDuck 23 hours ago
    
What is the type of options.AddBarcodeVerb and options.AddBlankPageVerb? Are they constant or do they change according to something? –  TopinFrassi 22 hours ago
    
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 –  thomasbtv 5 hours ago

3 Answers 3

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.

share|improve this answer
    
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 ? –  thomasbtv 4 hours ago

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.

share|improve this answer
    
Seems quite elegant to me and has much less syntax bloat compared to a class oriented solution. –  ChaosPandion 23 hours ago

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!

share|improve this answer
    
Why do you think inheritance is a better solution than delegates? –  ChaosPandion 21 hours ago
    
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.. –  TopinFrassi 19 hours ago
    
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. –  ChaosPandion 19 hours ago
    
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. –  thomasbtv 5 hours ago
    
@ChaosPandion, could you post an answer that would explain your point, I have difficulties understanding what you mean. –  TopinFrassi 2 hours ago

Your Answer

 
discard

By posting your answer, you agree to the privacy policy and terms of service.

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