views:

236

answers:

5

hey folks!

actually i refactor some portion of code. what i want to do is to initialize an object "Task" with an object "TaskArgument". let s say "TaskArgument" is abstract and "Task" implements a method "OnEnterTask(TaskArgument args)" and is sealed (for some special behavior of the existing system, which is out of scope).

old code:

public sealed class Task : SomeSystemBaseTask {
  private int accessMe; 
  private int meToo;

  public void OnEnterTask(TaskArgument args) {
    if (args is SimpleTaskArgument) {
      accessMe = ((SimpleTaskArgument)args).uGotIt;
      meeToo = 0;
    } else if (args is ComplexTaskArgument) {
      accessMe = ((ComplexTaskArgument)args).uGotItValue * ((ComplexTaskArgument)args).multiplier;
      meToo = ((ComplexTaskArgument)args).multiplier - 1;
    }
  }
}

what would be the best practise avoid the typecheck? my first stupud thought was:

public abstract class TaskArgument {
    internal public abstract Initialize(Task args);
}

public class SimpleTaskArgument : TaskArgument {
    public int uGotIt = 10;

    internal public Initialize(Task task){
        task.accessMe = uGotIt;
    }
}

public class ComplexTaskArgument : TaskArgument {
    public int uGotItValue = 10;
    public int multiplier = 10;

    internal public Initialize(Task task){
        task.accessMe = uGotItValue*multiplier;
        task.meToo = multiplier - 1;
    }
}

public sealed class Task : SomeSystemBaseTask {
    public int accessMe;
    public int meToo;

    public void OnEnterTask(TaskArgument args){
        args.Initialize(this);
    }
}

but then my "accessMe" is public and the "Initialize" method works only with "Task". so i moved the typechecking to another place (in future). is there any best practise or good design idea.

..."internal public"... mmhhmm?

another crazy idea was an inner class, but i dont like those and it make such a simple case more complex or don't:

public abstract class TaskArgument {
    internal public abstract Initialize(ITaskWrapper wrapper);
}

public class SimpleTaskArgument : TaskArgument {
    ...
}

public class ComplexTaskArgument : TaskArgument {
    ...
}

public interface ITaskWrapper {
    public int AccessIt { set; get; } 
    ...  
}

public sealed class Task : SomeSystemBaseTask {
    private int accessMe;
    ...

    class TaskWrapper : ITaskWrapper {
        ...
    }

    public void OnEnterTask(TaskArgument args){
        args.Initialize(new TaskWrapper(this));
    }
}

where is the best place for initialization when it is based on the given Type of the "TaskArgument"?

kindly excuse my bad english knowledge

greetings mo

+3  A: 
Paul Ruane
sounds great for one var/property.but in case of complex initialization? ComplexTaskArgument might be provide data for another var/propery on Task.i will add this case to my question.
mo
@Mo, then you need to rethink how you're modeling this. If you need to calculate something that requires state from Task and TaskArgument, then that calculation needs to occur in a seperate class that knows about both.
manu08
okay i think i goes along with good interface design.
mo
+1  A: 

OK, changed my answer a bit in light of the changing requirements appearing in the comments! (Sheesh, scope creep or what?!)

public class Task
{
    public int Variable1 { get; internal set; }
    public int Variable2 { get; internal set; }

    public void OnEnterTask(ITaskInitializer initializer)
    {
        initializer.Initialize(this);
    }
}

public interface ITaskInitializer
{
    void Initialize(Task task);
}

public class SimpleTaskInitializer : ITaskInitializer
{
    private int uGotIt = 10;

    public void Initialize(Task task)
    {
        task.Variable1 = uGotIt;
    }
}

public class ComplexTaskInitializer : ITaskInitializer
{
    private int uGotIt = 10;
    private int multiplier = 10;

    public void Initialize(Task task)
    {
        task.Variable1 = uGotIt;
        task.Variable2 = uGotIt * multiplier;
        // etc - initialize task however required.
    }
}
Ian Nelson
@Ian: You need to modify your Task.OnEnterTask method to take ITaskArgument instead of TaskArgument
manu08
+9  A: 

Use an interface.

public void OnEnterTask(TaskArgument args) { 
   if (args is SimpleTaskArgument) { 
      accessMe = ((SimpleTaskArgument)args).uGotIt; 
   } else if (args is ComplexTaskArgument) { 
      accessMe = ((ComplexTaskArgument)args).uGotItValue * ((ComplexTaskArgument)args).multiplier; 
   } 
} 

becomes

public void OnEnterTask(ITaskArgument args) { 
   accessMe = args.GetAccessMe();
} 

Then you have your classes implement ITaskArgument and implement the method for each class. In general, when you're doing something like this:

accessMe = ((ComplexTaskArgument)args).uGotItValue * ((ComplexTaskArgument)args).multiplier;

where you're accessing multiple properties on an object to perform a calculation, it usually makes sense to push that logic into the class itself.

manu08
i think this applies to a simple initialization
mo
@mo, yes my example works for simple initialization. But it looks like your creating a web of dependencies above. I think it's a smell to have the Task class take in a TaskArg object, and then call initialize on TaskArg where it passes itself (this). That's an awkward two-way dependency. Seperate your concerns! If you need to perform a calculation that uses state from Task and from TaskArgs, then leave Task and TaskArgs as state only classes (with minimal business logic). Then create another class that knows about both (but they don't know about it) and perform the calculations in there.
manu08
works for me :) that might be the cleanest way.
mo
A: 

Hey,

You could create overloads of Task as one option:

public class SimpleTask : Task
{
   public override void EnterTask(TaskArgument arg)
   {
      var s = (SimpleTaskArgument)arg;
   }
}

So each task type deals with an equivalent argument type. Or, you can move the logic to a TaskFactory with a static method that returns an int, and has the type checking argument there.

public static class TaskFactory
{
   public static int GetVal(TaskArgument arg)
   {
      if (args is SimpleTaskArgument) { 
        return ((SimpleTaskArgument)args).uGotIt; 
      } else if (args is ComplexTaskArgument) { 
        return ((ComplexTaskArgument)args).uGotItValue * ((ComplexTaskArgument)args).multiplier; 
      }
   }
}

Your interface implementation also would work; I wouldn't discount that... or define an abstract method within Taskargument, that each overrides to return the value.

HTH.

Brian
maybe the name "Task" irritates.i will define it as sealed.
mo
+1  A: 

I would create a public interface, which only exposes the Intialize method. Do your calculations in your derived classes e.g.

public interface ITaskArgument
{
    void Initialize(Task task);
}

public abstract class TaskArgument : ITaskArgument
{
    protected int _value;
    public class TaskArgument(int value)
    {
        _value = value;
    }

    public abstract void Initialize(Task task);
}

public class SimpleTaskArgument : TaskArgument, ITaskArgument
{
    public SimpleTaskArgument(int value)
       : base (value)
    {
    }

    public override void Initialize(Task task)
    {
        task.AccessMe = _value;
    }
}

public class ComplexTaskArgument : TaskArgument, ITaskArgument
{
    private int _multiplier;

    public ComplexTaskArgument(int value, int multiplier)
       : base (value)
    {
         _multiplier = multiplier;
    }

    public override void Initialize(Task task)
    {
        task.AccessMe = _value * _multiplier;
    }
}

public class Task
{
    public Task()
    {
    }

    public int AccessMe { get; set; }

    public void OnEnterTask(ITaskArgument args)
    {                         
        args.Initialize(this);                         
    }  
}

example

SimpleTaskArgument simpleArgs = new SimpleTaskArgument(10);
ComplexTaskArgument complexArgs = new ComplexTaskArgument(10, 3);
Task task = new Task();
task.OnEnterTask(simpleArgs);
Console.WriteLine(task.AccessMe); // would display 10
task.OnEnterTask(complexArgs);
Console.WriteLine(task.AccessMe); // would display 30
James