Command Pattern

Question

I'm trying to teach myself a new pattern, the command pattern, and came up with the following example. Looking for feedback.

public class BaseTool
{
    private List<ICommand> CommandHistory = new List<ICommand>();
    public string State = string.Empty; 

    public BaseTool()
    {

    }

    public virtual void ExecuteCommand(ICommand cmd)
    {

        if (cmd.CanExecute(this))
        {
            cmd.Execute(this);
            CommandHistory.Add(cmd);
        }
    }

    public string PrintHistory()
    {
        StringBuilder sb = new StringBuilder();

        foreach (ICommand cmd in CommandHistory)
        {
            sb.AppendLine(cmd.ToString());
        }

        return sb.ToString();
    }
}

public class Tool : BaseTool
{
    public Tool()
    {

    }
}

public interface ICommand
{
    void Execute(BaseTool t);
    bool CanExecute(BaseTool t);
}

public class PowerDownCommand : ICommand
{
    private DateTime _executeTime;

    public void Execute(BaseTool t)
    {
        if (!CanExecute(t))
        {
            throw new Exception("Error turning off the tool");
        }
        _executeTime = DateTime.Now;
        t.State = "OFF";
    }

    public bool CanExecute(BaseTool t)
    {
        return (t != null);
    }

    public override string ToString()
    {
        return _executeTime.ToLongTimeString() + " : was powered down";
    }
}

public class PowerUpCommand : ICommand
{
    private DateTime _executeTime;

    public void Execute(BaseTool t)
    {
        if (!CanExecute(t))
        {
            throw new Exception("Error turning on the tool");
        }

        _executeTime = DateTime.Now;
        t.State = "ON";

    }

    public bool CanExecute(BaseTool t)
    {
        return (t != null);
    }

    public override string ToString()
    {
        return _executeTime.ToLongTimeString() + " : was powered up";
    }
}

class Program
{
    static void Main(string[] args)
    {
        Tool t = new Tool();

        t.ExecuteCommand(new PowerUpCommand());

        Thread.Sleep(1000);

        t.ExecuteCommand(new PowerDownCommand());

        Console.WriteLine(t.PrintHistory());

        Console.Read();
    }

}

Answer 1

The Command Pattern's goal is to decouple the requestor object from the object performing the request. Your example does that.

However there is a small separation of responsibilities issue. Tool should not have to know to ask if ICommand can execute. That responsibility should be in the ICommand implementer. And because all interface members are public, CanExecute() should therefore be taken out of the interface.

You should document and code for the fact that ICommand.Execute() throws an exception, by wrapping the call in a try catch block (or somewhere upstream; dependent on your big picture design).

Finally, one of my pet peeves: the exception should give as much information as possible - like the fact that the Tool parameter was null. Use an exception appropriate for the situation: See ArguementException. Exceptions are for developers not end users so get down and dirty with details. Hint: Exception.Data

public interface ICommand
{
    void Execute(BaseTool t);
}

//ICommand implementer
public void Execute(BaseTool t)
{
    if (CanExecute(t))
    {
       _executeTime = DateTime.Now;
       t.State = "OFF";
     }else{
       throw new ArguementException("Error turning off the tool"); //this message sux
     }
}

//Tool class
public virtual void ExecuteCommand(ICommand cmd)
{
    try {
       cmd.Execute(this);
       CommandHistory.Add(cmd);
   }catch (Exception exception) {
       // whatever you do, do not just re-throw: "throw exception;".
   }
}