One of the things that I find jarring is that your model contains properties such as ExerciseFilter (the filter value), but the exerciseFilter (the actual filter) is defined in a completely different location.
This doesn't make logical sense. If your model is capable of supplying a given ID, and already knows what it will be using this ID for (since the model is the one deciding whether or not to use the filter), why not have your model manage your entire filter for you?
You're essentialy relying a form of codependency, which requires your developer(s) to simultaneously create a new filter in one class and a new filter value in another class. It would be better if the addition of a new filter only affects a single location, so that you don't start running into problems by forgetting to update the other location.
Your GetHistoryWorkoutExerciseByFilterAsync is not making any crucial decisions, it is simply acting based on how the model commands it to.
Another (minor) issue here is that you are creating filters before you're checking whether you will be using the filter. That's putting the cart before the horse.
I'm inclined to move the entire responsibility of filter creation to the model itself. It's already responsible for all the crucial parts of the filter (deciding whether to use it, supplying the actual values to use), so it might as well be responsible for supplying only the necessary filters (in a neatly created IEnumerable<Expression<...>>)
public async Task<List<HistoryWorkoutExerciseModel>> GetHistoryWorkoutExerciseByFilterAsync(ChartFilterModel model)
{
Expression<Func<HistoryWorkoutExercise, bool>> currentFilter = x => true;
foreach(var filter in model.GetFilters())
{
currentFilter.AndAlso(filter);
}
return _mapper.Map<List<HistoryWorkoutExerciseModel>>(await _repository.GetAsync(currentFilter, x => x.OrderByDescending(y => y.HistoryWorkout.TimeTakenDate), "Exercise,HistoryWorkout"));
}
That cleans up the code nicely. But of course, you need to add some new logic to the model, since it needs to do some more work now:
public IEnumerable<Expression<Func<HistoryWorkoutExercise, bool>>> GetFilters()
{
if(this.DateFilter > 0)
yield return (x => x.HistoryWorkout.TimeTakenDate >= this.StartDate && x.HistoryWorkout.TimeTakenDate <= this.EndDate);
if(this.ExerciseFilter > 0)
yield return (x => x.ExerciseId == this.ExerciseFilter);
}
Note that there are different ways to have your model return the list of filters. I prefer the yield return approach because it encapsulates all your filters in a single method, without the added code to add them in a list and return that list.
Some minor remarks
- You could use a similar approach for your
OrderBy expression. Have the model decide how it wants the data to be ordered (since ordering is usually a UI decision anyway).
model.ExerciseFilter should really be called ExerciseId. Arguably, you could call it ExerciseIdFilter, but you need the Id to be part of the name for clarity's sake.model.DateFilter is misusing an integer for the functional purpose of a boolean. You're better off using something like bool IsDateFilterRequired- You could do away with the additional
DateFilter/IsDateFilterRequired property altogether, and instead use a calculated property.
For example:
public DateTime? StartDate; //nullable
public DateTime? EndDate; //nullable
public bool IsDateFilterRequired
{
get
{
return this.StartDate.HasValue && this.EndDate.HasValue;
}
}
If you use this setup, you'll only be required to set the needed StartDate and EndDate, and the boolean will always be correct based on the values you've stored in the model.
Note that you could do a similar thing for the exercise Id:
public int? ExerciseIdFilter; //nullable
public bool IsExerciseFilterRequired
{
get
{
return this.ExerciseIdFilter.HasValue;
}
}
This results in slightly cleaner if blocks in the GetFilters() method.
