views:

992

answers:

5

I have the following code which I am are currently using .... Basically, this method assigns the correct boolean flag (TRUE/FALSE) for each Task. As more and more tasks need to be added .. I can see that the switch statement will have to grow to cater for every task.

There has to be an easier way ... to keep the method small.

Code: (forget naming convention, it has been changed for posting)

public ClassStructure.User AssignTaskStatusToUser(ClassStructure.User,
                                                  List<ClassStructure.Tasks> TaskStatus)
{
    foreach (ClassStructure.Tasks data in TaskStatus)
    {
        string Task_CallID = data.Task_Call_ID;

        switch (Task_CallID)
        {
            case ClassStructure.Tasks_CallIDs_Strings.TASK1:
                User.TASK1 = data.Task_Flag;
                break;

            case ClassStructure.Tasks_CallIDs_Strings.TASK2:
                User.TASK2 = data.Task_Flag;
                break;

            case ClassStructure.Tasks_CallIDs_Strings.TASK3:
                User.TASK3 = data.Task_Flag;
                break;
        }
    }

    return User;
}

ClassStructure.Tasks_CallIDs_Strings = String Representation of the Tasks

data.Task_Flag = boolean

User.TASKX = boolean

Any feedback is welcome. I am sure there is an easy solution.

+1  A: 

Could you have an array/list of tasks instead and use Task_CallID as an index into that?

e.g.

User.Tasks[Task_CallID] = data.Task_Flag;

If you must have them all as members there are other options:

  1. Maintain a mapping from Task_Call_ID to PropertyInfo reference and use that to set the correct property
  2. Use reflection to find the property based on the number bit (X) and set that property

Both of these are reflection based and a bit nasty.

Simon Steele
+7  A: 

For a lot of values like these, I would use a map something like this:

Dictionary<ClassStructure.Tasks_CallIDs_Strings, Task_Flag>

and retrieve values by mapping the CallIDs strings.

Edit:

As everyone can now see, the real problem of refactoring this example lies in refactoring User.TASKX. Making it a list should suffice - as it could then be indexed by the same string ClassStructure.Tasks_CallIDs_Strings

Elroy
+1, Something like this was going to be my suggestion as well.
Scott W
this only answers half the question though - notice that each branch of the switch is setting a different field
Chris Ballard
thats correct ... the reason for the switch is to correctly assign the correct property (Task) of the User class.
except for using Dictionary, this answer is not really relevant: it maps Tasks_CallIDs_Strings to Task_Flag instead of User's property assignment.
DK
funny thing, this answer still gets +9 so far
DK
I feel the User.TASKX is what needs to be refactored. How about making it a List.
Elroy
I agree with Elroy. The task list in User should be a list. Then it'd be extensible to any number of tasks.
kpollock
+2  A: 

Oh... Reconsider your naming scheme.

public delegate void TaskAssigner(User user, bool taskFlag)

IDictionary<string, TaskAssigner> taskAssigners = new Dictionary<string, TaskAssigner>();

...

taskAssigners.Add(ClassStructure.Tasks_CallIDs_Strings.TASK1, (u, t) => u.TASK1 = t;);
taskAssigners.Add(ClassStructure.Tasks_CallIDs_Strings.TASK2, (u, t) => u.TASK2 = t;);

...

foreach(ClassStructure.Tasks data in TaskStatus)
 taskAssigners[data.Task_Call_ID](user, data.Task_Flag);
Anton Gogolev
.. but this would still mean that we have to add a "taskAssigners.Add..." line for each new Task. I am looking for a method that will work without the need for maintenance .. when additional tasks are added over time.
+1  A: 

Why not make a Users Tasks structured as a list:

User Class

public List<ClassStructure.Tasks> Tasks {
    get; set;
}

Your Method becomes:

public void AssignTasks(User user, List<ClassStructure.Tasks> TaskStatus)    
{
    user.Tasks.AddRange(TaskStatus)   
}

Which is to say that you don't need the method at all. Your accessor then becomes running Find on a user's Tasks and checking the Tasks flag.

Gavin Miller
oh, yeah neater than mine! Forgot about AddRange...
kpollock
+1  A: 

I was thinking something like this - but maybe I missed the point of what it is all for?

public class User
{
    private Dictionary<string,Task> tasks;

    internal Dictionary<string,Task> Tasks
    {
      get { return tasks; }
      set { tasks = value; }
    }

    internal void AddTask(Task task)
    {
        tasks.Add(task.Task_Call_ID,task);
    }

    internal void AddTasks(List<Task> task)
    {
        foreach(Task task in Tasks)
        {
            tasks.Add(task.Task_Call_ID,task);
        }
    }
}

The Task class could have properties that allowed you to pass a function pointer (to the function that actually executes a task) if you needed that kind of flexibility - and you could add other methods like ExecuteTasks to User as well...

kpollock