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.
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
public async Task<List<HistoryWorkoutExerciseModel>> GetHistoryWorkoutExerciseByFilterAsync(ChartFilterModel model)
Expression<Func<HistoryWorkoutExercise, bool>> currentFilter = x => true;
foreach(var filter in model.GetFilters())
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
- You could do away with the additional
IsDateFilterRequired property altogether, and instead use a calculated property.
public DateTime? StartDate; //nullable
public DateTime? EndDate; //nullable
public bool IsDateFilterRequired
return this.StartDate.HasValue && this.EndDate.HasValue;
If you use this setup, you'll only be required to set the needed
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
This results in slightly cleaner
if blocks in the