1
\$\begingroup\$

Example of my method where I am chaining linq expressions based on model values provided:

public async Task<List<HistoryWorkoutExerciseModel>> GetHistoryWorkoutExerciseByFilterAsync(ChartFilterModel model)
{
    Expression<Func<HistoryWorkoutExercise, bool>> baseFilter = x => true && x.CreatedBy == model.CreatedBy;
    Expression<Func<HistoryWorkoutExercise, bool>> exerciseFilter = x => x.ExerciseId == model.ExerciseFilter;
    Expression<Func<HistoryWorkoutExercise, bool>> timeSpanFilter = x => x.HistoryWorkout.TimeTakenDate >= model.StartDate && x.HistoryWorkout.TimeTakenDate <= model.EndDate;

    if (model.ExerciseFilter > 0 && model.DateFilter == 0)
    {
        baseFilter = baseFilter.AndAlso(exerciseFilter);
    }

    if (model.ExerciseFilter == 0 && model.DateFilter > 0)
    {
        baseFilter = baseFilter.AndAlso(timeSpanFilter);
    }

    if(model.ExerciseFilter > 0 && model.DateFilter > 0)
    {
        baseFilter = baseFilter.AndAlso(timeSpanFilter).AndAlso(exerciseFilter);
    }

    return _mapper.Map<List<HistoryWorkoutExerciseModel>>(await _repository.GetAsync(baseFilter, x => x.OrderByDescending(y => y.HistoryWorkout.TimeTakenDate), "Exercise,HistoryWorkout"));
}

What's the best way to refactor this, as I am not the best fan of if conditional statements? Is there any other specific approach for this?

\$\endgroup\$
1
  • 2
    \$\begingroup\$ Not sure what true && adds \$\endgroup\$
    – paparazzo
    Oct 16, 2017 at 16:19

4 Answers 4

3
\$\begingroup\$

You have such properties as ExerciseFilter and DateFilter that I guess return respecitively the ExerciseId and (EndDate - StartDate).TotalMinutes or something similar. Thus, I find it would be much easier to understand your filter class if you renamed the ExerciseFilter to ExerciseId and replaced both properties with CanApplyExerciseFilter and CanApplyDateFilter. This would result in a much cleaner code:

if (model.CanApplyExerciseFilter && !model.CanApplyDateFilter)
{
    baseFilter = baseFilter.AndAlso(exerciseFilter);
}

if (!model.CanApplyExerciseFilter && model.CanApplyDateFilter)
{
    baseFilter = baseFilter.AndAlso(timeSpanFilter);
}

if (model.CanApplyExerciseFilter && model.CanApplyDateFilter)
{
    baseFilter = baseFilter.AndAlso(timeSpanFilter).AndAlso(exerciseFilter);
}

which would finally lead us to just two lines of code here:

baseFilter = 
    baseFilter
        .AndAlso(model.CanApplyExerciseFilter ? exerciseFilter : x => true)
        .AndAlso(model.CanApplyDateFilter ? timeSpanFilter : x => true);
\$\endgroup\$
2
  • \$\begingroup\$ What type is CanApplyExerciseFilter and CanApplyDateFilter? Boolean? If boolean why? I pas Exercise ID and start date and end date yes, which is int and datetimes. Are you saying there should be some logic behind that would set canApplyExerciseFilter to true if values are not 0? \$\endgroup\$
    – sensei
    Oct 17, 2017 at 9:12
  • \$\begingroup\$ @JohnMathilda yes, they are implemented e.g as public bool CanApplyExerciseFilter => ExerciseId > 0; \$\endgroup\$
    – t3chb0t
    Oct 17, 2017 at 9:28
1
\$\begingroup\$

You could abstract a ConditionalFilter to be used in a list with LINQ instead of writing many if statements:

public class ConditionalFilter
{
    private readonly Func<bool> myCondition;
    private readonly Func<HistoryWorkoutExercise, bool> myFilter;

    public ConditionalFilter(Func<bool> condition, Func<HistoryWorkoutExercise, bool> filter)
    {
        myCondition = condition;
        myFilter = filter;
    }

    public bool PassFilter(HistoryWorkoutExercise exercise) => myCondition() ? myFilter(exercise) : true;
}

public async Task<List<HistoryWorkoutExerciseModel>> GetHistoryWorkoutExerciseByFilterAsync(ChartFilterModel model)
{
    Func<HistoryWorkoutExercise, bool> baseFilter = x => true && x.CreatedBy == model.CreatedBy;
    Func<HistoryWorkoutExercise, bool> exerciseFilter = x => x.ExerciseId == model.ExerciseFilter;
    Func<HistoryWorkoutExercise, bool> timeSpanFilter = x => x.HistoryWorkout.TimeTakenDate >= model.StartDate && x.HistoryWorkout.TimeTakenDate <= model.EndDate;


    var filters = new ConditionalFilter[]
    {
        new ConditionalFilter(() => true, baseFilter),
        new ConditionalFilter(() => model.ExerciseFilter > 0 && model.DateFilter >= 0, exerciseFilter),
        new ConditionalFilter(() => model.ExerciseFilter >= 0 && model.DateFilter > 0, timeSpanFilter),
    };

    return _mapper.Map<List<HistoryWorkoutExerciseModel>>(await _repository.GetAsync(x => filters.All(f => f.PassFilter(x)), x => x.OrderByDescending(y => y.HistoryWorkout.TimeTakenDate), "Exercise,HistoryWorkout"));
}
\$\endgroup\$
1
\$\begingroup\$

Seems you could simplify it to just be this. Takes 3 if to 2 and I feel reads better.

if (model.ExerciseFilter > 0 && model.DateFilter >= 0)
{
    baseFilter = baseFilter.AndAlso(exerciseFilter);
}

if (model.DateFilter > 0 && model.ExerciseFilter >= 0)
{
    baseFilter = baseFilter.AndAlso(timeSpanFilter);
}
\$\endgroup\$
4
  • 1
    \$\begingroup\$ I think it should be model.ExerciseFilter > 0 && model.DateFilter >= 0 and model.ExerciseFilter >= 0 && model.DateFilter > 0!? \$\endgroup\$
    – JanDotNet
    Oct 16, 2017 at 16:34
  • \$\begingroup\$ Why? His code is if ExerciseFilter is greater than zero to add the ExecciseFilter and if Datafilter is greater than zero to add the datafilter. Since Expression trees make new expressiontree each time you add them you can just chain them together. That's what he's doing when both are greater than zero \$\endgroup\$ Oct 16, 2017 at 17:03
  • 1
    \$\begingroup\$ Because if DataFilter is less then 0, exerciseFilter must not be applied and if ExerciseFilter is less then 0, timeSpanFilter must not be applied. \$\endgroup\$
    – JanDotNet
    Oct 16, 2017 at 17:30
  • \$\begingroup\$ True I didn't think of negative numbers. \$\endgroup\$ Oct 16, 2017 at 17:46
1
\$\begingroup\$

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.

\$\endgroup\$

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service, privacy policy and cookie policy

Not the answer you're looking for? Browse other questions tagged or ask your own question.