1

I am using Command Pattern to execute actions and so I developed many command classes (CommandA, CommandB, etc.). Now I need a SuperCommand that receives a collection of data from legacy code, loops through it and calls one of previous defined commands depending on data type field.

The pseudo code of the SuperCommand should be something like:

class SuperCommand: public ICommand {
    public:
        SuperCommand(LegacyDataList datas): datas_(datas) {}

        void execute() {
            foreach(LegacyData data : datas_) {
                switch(data.type) {
                    case A:
                        CommandA(data).execute();
                    case B:
                        CommandB(data).execute();
                    default:
                        CommandC(data).execute();
                }
            }
        }
    private:
        LegacyDataList datas_;
}

The question is:

I am Learning TDD and I would like to code execute methd of class SuperCommand starting from tests. But I cannot figure out how to test the foreach-switch logic to verify that the correct command is executed.

The only idea I had is to inject commands in SuperCommand constructor so that I could mock them, but it seems a poor solution given that the meaning of the class is choosing between CommandA, CommandB and CommandC (NO other implementations will ever be passed to this class).

Are there better solutions? Should I change the design of SuperCommand?

0

Responsibility of SuperCommand class is to execute commands based on the provided data.

ICommand :: execute method doesn't return value, so you can not assert against it, then you need to check that some state have been updated properly. But since you uses different commands and SuperCommand doesn't need to care about what state have been updated - you can just check that correct implementation of ICommand is executed.

Then you will face a problem with writing tests first, because you want/do create instances of actual commands inside SuperCommand::execute() method. That mean you cannot mock their behaviour - you need test their actual code. Which mean you need to test code which SuperCommand not responsible for.

Because command implemented so that required data passed through constructor, but you cannot use constructor because of the tests. Then you need introduce one more layer of abstraction which will be responsible for creating correct instance of the command - CommandFactory

So now - seems like SuperCommand responsibility become to loop all data, use factory for creation command and executing it.

1

Consider extending ICommand to include a method canHandle accepting as argument an instance of LegacyData.

Then in your SuperCommand class, keep a list of ICommand instances to call. For each LegacyData that you come across, call canHandle passing the current instance of LegacyData, and if it returns true, you call it and break.

You can therefore test it by simply instantiating your ICommand list with stubs containing only ICommands which do nothing and ICommands which shouldn't be run and throw an exception if they are.

Doing it this way also uncouples SuperCommand from the commands it runs in a pinch. You could simply add a new method addCommand that lets the caller decide which commands are attributed to SuperCommand. If anything you can create a extended version of SuperCommand which already adds specific ICommand instances that you're using (CommandA, CommandB, and CommandC in your example). Yes, while it's true you're only using these three commands, it's also true that SuperCommand doesn't really care that they are CommandA, CommandB, and CommandC being run, and so by doing it this way, you make the class both flexible and easily tested.

  • Thanks for the response! Isn't your solution pretty the same of passing stubs via constructor? – Marco Stramezzi Jun 22 '17 at 9:35
  • @MarcoStramezzi For the most part yeah. You have to make the ICommand instances dynamic in order to test with stubs. – Neil Jun 22 '17 at 9:45
1

First, let me say that I really do not see a reason why SuperCommand should implement ICommand interface. Unless this is part of some bigger picture that is not mentioned in this question, it is totally unnecessary. SuperCommand here serves purely as a wrapper around different command classes being executed.

Next, what @Neil described is very, very similar to the Chain of Responsibility design pattern. What this means is that you should implement CommandA, CommandB, CommandC in such a way that their Execute methods are executed only for proper data types being sent to them. That way, you can chain your commands and just let them worry about whether something should be executed or not. SuperCommand would just be a wrapper and host for your command classes. Something like this:

public class LegacyData
{
    public string Type;
}

public abstract class Command
{
    protected abstract bool DataCheck(LegacyData data);
    public abstract void Execute(LegacyData data);       
    public Command Next { protected get; set; }

}

class CommandA : Command
{
    protected override bool DataCheck(LegacyData data)
    {
        return data.Type == "A";
    }
    public override void Execute(LegacyData data)
    {
        if (DataCheck(data))
        {
            //Do something with data

            //This return statement is here only because the conditions in this example
            //are mutually exclusive. In case there is data that can be processed by multiple
            //commands, the return statement would not be put here.
            return;
        }

        if (Next != null)
        {
            Next.Execute(data);
        }
    }
}

class CommandB : Command
{
    protected override bool DataCheck(LegacyData data)
    {
        return data.Type == "B";
    }
    public override void Execute(LegacyData data)
    {
        if (DataCheck(data))
        {
            //Do something with data

            //This return statement is here only because the conditions in this example
            //are mutually exclusive. In case there is data that can be processed by multiple
            //commands, the return statement would not be put here.
            return;
        }

        if (Next != null)
        {
            Next.Execute(data);
        }
    }
}

class CommandC : Command
{
    protected override bool DataCheck(LegacyData data)
    {
        return data.Type != "A" && data.Type != "B";
    }
    public override void Execute(LegacyData data)
    {
        if (DataCheck(data))
        {
            //Do something with data

            //This return statement is here only because the conditions in this example
            //are mutually exclusive. In case there is data that can be processed by multiple
            //commands, the return statement would not be put here.
            return;
        }

        if (Next != null)
        {
            Next.Execute(data);
        }
    }
}

class SuperCommand
{
    private List<LegacyData> datas;
    public SuperCommand(List<LegacyData> datas){ this.datas = datas; }

    private Command initialCommand;

    public SuperCommand()
    {
        CommandA commandA = new CommandA();
        CommandB commandB = new CommandB();
        CommandC commandC = new CommandC();

        commandA.Next = commandB;
        commandB.Next = commandC;

        initialCommand = commandA;
    }

    public void Execute()
    {
        foreach (LegacyData data in datas)
        {
            initialCommand.Execute(data);
        }
    }       
}

With this approach, you would have completely decoupled SuperCommand from the logic in Command classes. The only thing tying the SuperCommand with concrete Command classes would be their instantiation and linking in the constructor. The logic about what command will be executed would be relocated down to the Command classes, which you could easily test then.

  • Even with Chain of Responsibility, my problem is still there... How could I TDD SuperCommand::execute()? – Marco Stramezzi Jun 22 '17 at 10:32
  • In your question, you have the following sentence: "But I cannot figure out how to test the foreach-switch logic to verify that the correct command is executed." With this solution, you do not have foreach-switch in SuperCommand::execute, but just a simple foreach loop, which, honestly, does not even need to be tested. Instead, you write unit tests for CommandA::Execute, CommandB::Execute, etc. to verify that they are indeed executed when they need to be. – Vladimir Stokic Jun 22 '17 at 11:07
0

Commands respond the user actions (either a person or another system). They should not be aware of nor execute other commands. Strategy pattern is more suitable to your scenario. CommandA, CommandB and CommandC become strategies and you can implement them like so:

interface IStrategy
{
    void Execute(LegacyData data);
    bool AppliesTo(LegacyData data);
}

class StrategyA
{
    public void Execute(LegacyData data)
    {
        // ...
    }

    public bool AppliesTo(LegacyData data)
    {
        return data.type == A;
    }
}

This way you can unit test the behavior of your strategies and the selection process.

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.