strategy for choosing proper object and proper method
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
{
开发者_JAVA百科 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?
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:
- A parameterless public (or at least internal) constructor for FilterBase and the derived classes.
- A virtual method named
Initialize
that takes a string on FilterBase. - An IsNew and IsEnded static function on your
Filter
class so that theFetch
method can determine which function you can use. Another solution would be an additional delegate parameter toFetch
to determine which method should be called.
How can you improve the rest of the code?
- Stop using underscores in class and method names (and field names).
- Use Pascal Case for method names (so NewIssues instead of new_issues)
- Make your base class abstract unless there is compelling reason otherwise.
- Find a more definitive means of determining "new" and "ended". Perhaps an additional enum parameter?
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;
}
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");
精彩评论