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 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!

share|improve this question

1 Answer 1

up vote 6 down vote accepted

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));
share|improve this answer
    
So, basically you're saying that it's better to couple the action and its parameters? I'm not completely sure about that. –  svick Jan 16 '13 at 16:14
    
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). –  almaz Jan 16 '13 at 16:24
    
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? –  Johnny Mopp Jan 16 '13 at 16:34
    
Why would you make ICommand a class? –  Jeff Vanzella Jan 16 '13 at 19:30
    
@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. –  almaz Jan 17 '13 at 12:56

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.