5
\$\begingroup\$

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();
    }

}
\$\endgroup\$
2
  • \$\begingroup\$ Just curious, why do you have parentheses around (t != null)? And I'm not sure how much you gain in readability by having the CanExecute() method. \$\endgroup\$
    – Zolomon
    Mar 16, 2012 at 19:00
  • \$\begingroup\$ Zolomon - habit \$\endgroup\$
    – Scott
    Mar 19, 2012 at 4:10

1 Answer 1

6
\$\begingroup\$

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;".
   }
}
\$\endgroup\$

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service and acknowledge that you have read and understand our privacy policy and code of conduct.

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