4
\$\begingroup\$

How can I improve up this code?
Is it ok to write code using Invoke and Action so liberally or is this bad?

Performance is not an issue as I'm not using Invoke 50,000x in a a row (a lot of other things will be done in-between, making the performance hit null).

The primary goal is to run my list of tasks either by Item first or task first. The secondary goal is to NOT write a massively long method where each method is written out in each way they can be completed. Doing so makes adding new tasks later much easier.

Code: (Updated with pseudo extras)

public class TaskObject
{
    public bool RunPerItem = true; //PLACEHOLDER, normally derived externally from view model
    public bool RunPerTask = false; //PLACEHOLDER, normally derived externally from view model

    public Dictionary<string, Item>() TaskItems = new Dictionary<string, Item>()
    {
        { "ItemA", new Item() {} }, 
        { "ItemB", new Item() {} }, 
        { "ItemC", new Item() {} }, 
    }   

    private List<Action<TaskObject, Item>> Tasks = new List<Action<TaskObject, Item>>()
    {
        (TO, I) => { TO.TaskA(I); },
        (TO, I) => { TO.TaskB(I); },
        (TO, I) => { TO.TaskC(I); }
    };

    public void RunTasks()
    {
        //RunPerItem & RunPerTask are type `bool`
        if (this.RunPerItem) 
        {
            foreach (var I in this.TaskItems)
            {
                foreach (var T in this.Tasks)
                {
                    T.Invoke(this, I.Value); 
                }
            }
        }
        else if (this.RunPerTask)
        {
            foreach (var T in this.Tasks)
            {
                foreach (var I in this.TaskItems)
                {
                    T.Invoke(this, I.Value); 
                }
            }
        }
    }  

    public void TaskA(Item I) { /*...*/ }
    public void TaskB(Item I) { /*...*/ }
    public void TaskC(Item I) { /*...*/ }
}

public class Item 
{
    public string ItemName { get; set; }
} 

Thanks for any advice/help!!!

Edit/Clarification/Extra Details:

The above code describes 2 different loops with different orders of task completion.

Example Orders:
this.RunPerItem
ItemA
-TaskA
-TaskB
-TaskC

ItemB
-TaskA
-TaskB
-TaskC

this.RunPerTask
ItemA
-TaskA

ItemB
-TaskA

\$\endgroup\$
4
  • \$\begingroup\$ Let me check if I understand the idea. You have 2 concepts: tasks and items. You say "This task should be done against this item". The relations between tasks and items are many-to-many. Single task can be applied to multiple items and single item can be processed by multiple tasks. Depending on some conditions, you either say "I want all the tasks required for this item to be ran" or "I want this task to process all the items it should". Is that correct? \$\endgroup\$ Jul 27 '11 at 18:37
  • \$\begingroup\$ This code isn't even close to a compileable state. I'm not sure what the "this" object refers to in the T.Invoke() calls. Is the RunTasks() method actually on a TaskObject? \$\endgroup\$
    – John Kraft
    Jul 27 '11 at 21:29
  • \$\begingroup\$ @John Kraft It's not suppose to be. I'm just asking for suggestions on refactoring the loop using different approaches. Not my entire code. this refers to the object that contains RunTasks(). I didn't include the entire object as it's pointless to my example. I'll update my code with some extra pseudo code. \$\endgroup\$
    – PiZzL3
    Jul 28 '11 at 0:25
  • \$\begingroup\$ @loki2302 You have it kinda right. The 2 loops only change the order in which tasks are done on each item. The number of items and tasks is irrelevant. In my example, it will either run task A on each item, then move to B, C, etc.. or do all tasks on 1 item (A, B, C, etc..) and then move to the next item and repeat. As you can see in my example code, the second set of foreach statements are ordered the opposite of the first set ((foreach item -> foreach task), (foreach task -> foreach item)), and the task always processes the item the same way in the end (T.Invoke(this, I.Value);). \$\endgroup\$
    – PiZzL3
    Jul 28 '11 at 0:38
4
\$\begingroup\$

I don't know if you can, depending on your real world needs... but if you can convert the TaskItems Dictionary to a List of KeyValuePairs like this...

public List<KeyValuePair<string, Item>> TaskItems = new List<KeyValuePair<string, Item>>
{
    new KeyValuePair<string, Item>( "ItemA", new Item() {} ),
    new KeyValuePair<string, Item>( "ItemB", new Item() {} ), 
    new KeyValuePair<string, Item>( "ItemC", new Item() {} ), 
};

Then you can use the ForEach() exstension method to reduce the loop code to...

if (this.RunPerItem) 
{
    TaskItems.ForEach(I => Tasks.ForEach(T => T(this, I.Value)));            
}
else if (this.RunPerTask)
{
    Tasks.ForEach(T => TaskItems.ForEach(I => T(this, I.Value)));
}

If you couple that with @Steven Jueris' idea of splitting them into their own methods, your code will be very clean.

\$\endgroup\$
5
  • \$\begingroup\$ That is beautiful! I thought there was some fancy linq/lambda/extension method way of doing this. I just couldn't think of it. I still have trouble thinking of my code in that manner as I've been writing out all my code the long way like above for years. Thanks! \$\endgroup\$
    – PiZzL3
    Jul 28 '11 at 20:03
  • \$\begingroup\$ I think I could extend the dictionary class to support the ForEach extension method too.. That should be easy to find on google. \$\endgroup\$
    – PiZzL3
    Jul 28 '11 at 20:07
  • \$\begingroup\$ Yes. There's a gazillion implementations of it on the net. The only difference between the list and dictionary, the way you are using it, is that the dictionary won't allow you to add the same key/value pair twice. It's a good measure of protection if you cannot control what gets put into the collection. However, if you have full control over the collection and don't have to worry about that, then there's no harm in using the list and the built in method. \$\endgroup\$
    – John Kraft
    Jul 28 '11 at 20:10
  • 1
    \$\begingroup\$ Personally, I'd stay away from using the List<T>.ForEach() method. It doesn't really make a foreach looping construct look much cleaner. In some ways, it unnecessarily complicates it. \$\endgroup\$ Jul 28 '11 at 20:16
  • \$\begingroup\$ @Jeff Mercado - normally I would agree. In more complicated usage, it can complicate the code, as well as, cause difficulty debugging. However, IMO, this simple usage lends itself well to the construct. \$\endgroup\$
    – John Kraft
    Jul 28 '11 at 20:27
1
\$\begingroup\$

Instead of calling your delegates by using Invoke you can call them using the following easier formatting:

Action someAction = () => {};
someAction();

Delegate invocations aren't that much slower than ordinary method calls. No need to worry about performance.

\$\endgroup\$
4
  • \$\begingroup\$ This sounds cool, but is a bit above me right now. How do I apply this to the current List of Actions I currently have in the code above? From what I understand of your example code, I would have to make them all fields which would kill the looping over them idea as far as I can see/understand. Thanks! \$\endgroup\$
    – PiZzL3
    Jul 28 '11 at 1:05
  • \$\begingroup\$ T( this, I.Value ); ... Why would there be any difference? ;p \$\endgroup\$ Jul 28 '11 at 11:41
  • \$\begingroup\$ Ahhh cool.. I didn't know you could do that.. interesting... Is this syntactic sugar or does it behave slightly different than using T.Invoke(..? \$\endgroup\$
    – PiZzL3
    Jul 28 '11 at 12:00
  • \$\begingroup\$ I'm pretty certain it's syntactic sugar. When you want to call the delegate asynchronously however, you can use BeginInvoke and EndInvoke. \$\endgroup\$ Jul 28 '11 at 13:59
0
\$\begingroup\$

Simple thing I see is for readability i'd place the inside of each top ifs into a seperate method with an explicit name. And I always like returning immediately (often times the method itself) then you don't need the else (your second if).

public void RunTasks()
{
    if (this.RunPerItem) 
    {
        return RunTasksForEachItem();
    }

    return RunEachTask();
} 
\$\endgroup\$
2
  • \$\begingroup\$ Good idea with the explicit method idea. I'll look into the return idea. I'm not sure it'll work with my real code. \$\endgroup\$
    – PiZzL3
    Jul 28 '11 at 1:00
  • \$\begingroup\$ Ahh... I see now. I implemented the method separation idea, but the return idea would break the rest of my code (interaction with my view, which isn't in the code above). \$\endgroup\$
    – PiZzL3
    Jul 28 '11 at 20:01

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.