4
\$\begingroup\$

I am trying to create a class to queue up a series of commands. (Not really a queue as events happen based on time). Each command has a callback (Action) that get's called. However, each Action has a different set of parameters (signature), so I am using an object[] to pass them and then have to do lots of casting.

Example: the Queue class:

class RfxQueue {
    public enum CMD {DISPLAY, REPLACE_TMPLT, SCALE_DVE, etc };
    private Dictionary<CMD, Action<object []>> actions = new Dictionary<CMD,Action<object[]>>();
    // Struct to hold command info
    public struct QItem
    {
        public CMD cmd;           // The CMD to exec
        public long ttd;          // Time to be executed
        public object[] param;    // Parameters to Action
    }
    private List<QItem> q = new List<QItem>();   // The queue of events

    // Create a new callback
    public void addAction(CMD cmd, Action<object []> action)
    {
        actions.Add(cmd, action);
    }

    // Add to Q
    public void pushEvent(CMD Cmd, float Delay, params object[] Param)
    {
        q.Add(new QItem() { cmd = Cmd, param = Param, ttd = DateTime.Now.AddMilliseconds(Delay * 1000).Ticks });
    }

    // This is actually done in a thread. Simplified here
    private void execEvent()
    {
        QItem item;
        // item is pulled from q here
        actions[item.cmd].Invoke(item.param);
    }
}

And the owner class creates events like this:

// "q" is a RfxQueue and the internal Invoke is required to run on main thread
q.events.Add(RfxQueue.CMD.SCALE_DVE, new Action<object[]>(o =>
{
    dispatcher.Invoke(new Action<string, string, bool>((a,b,c) =>
    {
        _cgManager.ScaleDVE(a, b, c);
    }), (string)o[0], (string)o[1], (bool)o[2]);
}));
q.events.Add(RfxQueue.CMD.PLAY_ANIMATION, new Action<object[]>(o =>
{
    dispatcher.Invoke(new Action<VideoTemplate, string>((v, s) => {
        _cgManager.playAnimation(v, s);
    }), (VideoTemplate)o[0], (string)o[1]);
}));

Lastly, an event is added like this:

// Play animation "inserton" in 3 seconds
q.add(RfxQueue.CMD.PLAY_ANIMATION, template, 3f, "inserton");

Is there a way to do this without all that casting? Thanks!

\$\endgroup\$

1 Answer 1

7
\$\begingroup\$

Starting from minor issues - please follow naming conventions (variables should be camelCased and methods PascalCased, do add access modifiers, give variables meaningful names).

Your design breaks Open-Closed principle: Queue knows all the possible commands it can handle. In case when you need to add a new command you'll have to change the queue rather than add new code.

Also it breaks Single Responsibility Principle since queue knows not only about scheduling the execution but also serves as a repository for all possible actions to be run for commands.

Both issues as well as those you have mentioned (type casting, different signatures) can be easily solved if you create a separate class per command type. Command will encapsulate parameters required for execution as well as the action to be executed:

public interface ICommand
{
    void Execute();
}

public class ScaleDveCommand : ICommand
{
    public CgManager CgManager { get; set; }
    public string FirstString { get; set; } //TODO:Rename to meaningful name
    public string SecondString { get; set; } //TODO:Rename to meaningful name
    public bool SomeBool { get; set; } //TODO:Rename to meaningful name

    public void Execute()
    {
        dispatcher.Invoke(() => CgManager.ScaleDVE(FirstString, SecondString, SomeBool));
    }
}

public class PlayAnimationCommand : ICommand
{
    public CgManager CgManager { get; set; }
    public VideoTemplate VideoTemplate { get; set; }
    public string SomeMeaningfulNameHere { get; set; } //TODO:Rename to meaningful name

    public void Execute()
    {
        dispatcher.Invoke(() => { CgManager.playAnimation(VideoTemplate, SomeMeaningfulNameHere); });
    }
}

And your "queue" class will look like:

public class RfxQ
{
    private readonly SortedList<long, ICommand> _queue = new SortedList<long, ICommand>();   // The queue of events

    public void PushEvent(ICommand command, TimeSpan delay)
    {
        _queue.Add(DateTime.Now.Add(delay).Ticks, command);
    }

    // This is actually done in a thread. Simplified here
    private void ExecEvent()
    {
        ICommand command;
        // item is pulled from _queue here
        command.Execute();
    }
}

And usage will look like:

q.pushEvent(new PlayAnimationCommand
    {
        CgManager = _cgManager,
        VideoTemplate = template,
        SomeMeaningfulNameHere = "inserton"
    }, TimeSpan.FromSeconds(3));
\$\endgroup\$
5
  • \$\begingroup\$ So, basically you're saying that it's better to couple the action and its parameters? I'm not completely sure about that. \$\endgroup\$
    – svick
    Commented Jan 16, 2013 at 16:14
  • \$\begingroup\$ OOP defines class as a structure that combines state and behavior, so I don't see issue in coupling action (behavior) with parameters of that action (state). \$\endgroup\$
    – almaz
    Commented Jan 16, 2013 at 16:24
  • \$\begingroup\$ Thanks for the nice answer. The only changes I will make are 1) making CgManager and dispatcher static vars in ICommand (making ICommand a class), and 2) have each command accept the parameters in their constructors. These will shorten calles to PushEvent. Will this break OO design? \$\endgroup\$
    – 001
    Commented Jan 16, 2013 at 16:34
  • \$\begingroup\$ Why would you make ICommand a class? \$\endgroup\$ Commented Jan 16, 2013 at 19:30
  • \$\begingroup\$ @JohnnyMopp It's hard to suggest the best approach actually since I don't know how CgManager and Dispatcher are managed. I generally dislike static fields (except static readonly ones) as it is a possible headache in multithreading environment as well as makes unit testing harder. Accepting parameters in constructor is fine. \$\endgroup\$
    – almaz
    Commented Jan 17, 2013 at 12:56

Your Answer

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

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