strategy for choosing proper object and proper method

Question

in the code below at first if statements block (there will be more than just "worker" condition, joined with else if) i select proper filter_object. After this in the same conditional block i select what filter should be applied by filter object. This code is silly.

public class Filter
{
    public static List<data.Issue> fetch(string type, string filter)
    {
        Filter_Base filter_object = new Filter_Base(filter);

        if (type == "worker")
        {
            filter_object = new Filter_Worker(filter);
        }
        else if (type == "dispatcher")
        {
            filter_object = new Filter_Dispatcher(filter);
        }

        List<data.Issue> result = new List<data.Issue>();

        if (filter == "new")
        {
            result = filter_object.new_issues();
        }
        else if (filter == "ended")
        {
            result = filter_object.ended_issues();
        }

        return result;
    }
}

public class Filter_Base
{
    protected string _filter;

    public Filter_Base(string filter)
    {
        _filter = filter;
    }

    public virtual List<data.Issue> new_issues()
    {
        return new List<data.Issue>();
    }

    public virtual List<data.Issue> ended_issues()
    {
        return new List<data.Issue>();
    }
}

public class Filter_Worker : Filter_Base
{
    public Filter_Worker(string filter) :
        base(filter)
    { }

    public override List<data.Issue> new_issues()
    {
        return (from i in data.db.GetInstance().Issues
                where (new int[] { 4, 5 }).Contains(i.RequestStatusId)
                select i).Take(10).ToList();
    }
}

public class Filter_Dispatcher : Filter_Base
{
    public Filter_Dispatcher(string filter) :
        base(filter)
    { }
}

it will be used in some kind of:

Filter.fetch("worker", "new");

this code means, that for user that belongs to role "worker" only "new" issues will be fetched (this is some kind of small and simple CRM). Or another:

Filter.fetch("dispatcher", "ended"); // here we get finished issues for dispatcher role

Any proposals on how to improve it?

Answer 1

DI is a possibilty if there are loads of FilterBase derivatives. Depending on your weapon of choice code looks similar to this:

ioc
  .RegisterType<IDataStrategy, FilterWorker>("worker")
  /// more filters
  .RegisterType<IDataStrategy, FilterDispatcher>("dispatcher");

Alternatively a factory object could at least refactor your code, but I digress...

while your at it, lose the base class and implement an interface:

public interface IDataStrategy
{
    IEnumerable<Data.Issue> FetchNew();
    IEnumerable<Data.Issue> FetchEnded(); 
}

Use an enum as suggested:

public enum FilterType 
{
    New,
    Ended
}

Now in your Fetch function

public IEnumerable<Data.Issue> Fetch(string dataType, FilterType filterType)
{ 
    var strategy = ioc.Resolve<IDataStrategy>(dataType);
    IEnumerable<Data.Issue> results = null;
    switch(filterType)
    {
        case FilterType.New:
            results = strategy.FetchNew();
        default:
            results = strategy.Ended();
    }
    return results; 
}

Answer 2

This might seem like a long way, but I would make use of attributes and reflection to try to achieve this.

Something like

Attributes:

class FilterType : Attribute
{
    public string Filter;
    public FilterType(string filter)
    {
        Filter = filter;
    }
}

class FilterMethod : Attribute
{
    public string Filter;
    public FilterMethod(string filter)
    {
        Filter = filter;
    }
}

Classes with attribute markers

public class Filter_Base
{
    protected string _filter;

    public Filter_Base(string filter)
    {
        _filter = filter;
    }

    [FilterMethod("new")]
    public virtual List<string> new_issues()
    {
        return new List<string>();
    }
}
[FilterType("Worker")]
public class Filter_Worker : Filter_Base
{
    public Filter_Worker(string filter) :
        base(filter)
    { }

    public override List<string> new_issues()
    {
        return new List<string>();
    }
} 

Filter Method

private static List<string> GetInstanceList(string type, string filter)
{
    //get all classes implementing FilterType Attribute
    var dict = AppDomain.CurrentDomain.GetAssemblies().
                SelectMany(x => x.GetTypes()).
                Where(x => x.GetCustomAttributes(typeof(FilterType), false).Length > 0).
                Select(x => new { ((FilterType)x.GetCustomAttributes(typeof(FilterType), false)[0]).Filter, x }).
                ToDictionary(x => x.Filter);

    Filter_Base instance = (Filter_Base)Activator.CreateInstance(dict[type].x, filter);

    var methods = instance.GetType().GetMembers().
                    Where(x => x.GetCustomAttributes(typeof(FilterMethod), true).Length > 0).
                    Select(x => new { ((FilterMethod)x.GetCustomAttributes(typeof(FilterMethod), true)[0]).Filter, x }).
                    ToDictionary(x => x.Filter);


    return (List<string>)instance.GetType().GetMethod(methods[filter].x.Name).Invoke(instance, null);
}

And lastly call

List<string> instance = GetInstanceList("Worker", "new");

Answer 3

I'm assuming you are asking how you can you trim up the Fetch method. I'd use generics

public static List<data.Issue> Fetch<T>( string filter ) where T : FilterBase, new()
{
    var filterBase = new T();
    filterBase.Initialize( filter );

    List<data.Issue> result;
    if ( IsNew( filter ) )
        result = filterBase.NewIssues();
    else if ( IsEnded( filter ) )
        result = filterBase.EndedIssues();
    else
        result = new List<data.Issue>();

    return result;
}

This requires:

1. A parameterless public (or at least internal) constructor for FilterBase and the derived classes.
2. A virtual method named Initialize that takes a string on FilterBase.
3. An IsNew and IsEnded static function on your Filter class so that the Fetch method can determine which function you can use. Another solution would be an additional delegate parameter to Fetch to determine which method should be called.

How can you improve the rest of the code?

1. Stop using underscores in class and method names (and field names).
2. Use Pascal Case for method names (so NewIssues instead of new_issues)
3. Make your base class abstract unless there is compelling reason otherwise.
4. Find a more definitive means of determining "new" and "ended". Perhaps an additional enum parameter?