Filter data with optional parameters

Question

I would make a filter to filter my data using next parameters:

1. First name
2. Last name
3. Language
4. Profile
5. A minimum date of registration
6. A maximum date of registration
7. Success

All filter options are not required for the user. Here could you find my C# code.

private IEnumerable<Registration> Filter(string voornaam, // first name
 string achternaam, // last name
 int? language, 
 int? profiel, // profile
 DateTime? vanaf, // min date
 DateTime? tot, // max date
 bool? geslaagd) // success
{
 IEnumerable<Registration> all = All(); // <-- this gets all registrations from 
 // the database.
 if (!string.IsNullOrEmpty(voornaam))
 {
 all = (from r in all
 where r.FirstName.ToLower().StartsWith(voornaam.ToLower().Trim())
 select r).ToList();
 }
 if (!string.IsNullOrEmpty(achternaam))
 {
 all = (from r in all
 where r.LastName.ToLower().StartsWith(achternaam.ToLower().Trim())
 select r).ToList();
 }
 if (language != null)
 {
 all = (from r in all
 where r.LanguageId == language.Value
 select r).ToList();
 }
 if (profiel != null)
 {
 all = (from r in all
 where r.ProfileId == profiel.Value
 select r).ToList();
 }
 if (vanaf != null)
 {
 all = (from r in all
 where r.RegistrationDate >= vanaf.Value
 select r).ToList();
 }
 if (tot != null)
 {
 all = (from r in all
 where r.RegistrationDate <= tot.Value
 select r).ToList();
 }
 if (geslaagd != null)
 {
 all = (from r in all
 where r.Success == geslaagd.Value
 select r).ToList();
 }
 return all;
}

The function All() gets all the registrations from the database. Next I check if the attributes are not null (or empty for the strings) and if true I set the variable all equal to the return values of the linq query's. After all I return all and load my view.

This code works for all possibilities and got a low fail percentage. Of course my code is too long and will make this code shorter. How could I do this?

• Mast: It's really not enough to write an answer for, but be consistent with your variable naming. Now you have part in Dutch and part in English. Go English all the way instead.

Answer 1

IEnumerable<Registration> all = All(); // <-- this gets all registrations from 
 // the database.

This looks like a really bad idea. Why are you getting all data from the database first and not filter it before retrieving?

where r.FirstName.ToLower().StartsWith(voornaam.ToLower().Trim())

Most databases are case insensitive so you wouldn't need this formatting adjustment if you used server side filtering.

On the client it's better to use the StrigComparer.XIgnoreCase where the X is either the InvariantCulture or Ordinal then ToUpper/Lower because on the one hand it better communicates the intention that you are performing a case-insensitive comparison (which is not that obvious if you use ToUpper/Lower - you might be just fixing the case for whatever reason) and on the other hand you don't have to change the case twice which might introduce a new bug if you call ToUpper for the first string and ToLower for the other one or forget it altogether.

If you use EF or any other ORM you should filter the data on the server and not after downloading everything. With large databases this would be such a waste of resources.

If All is backed by Entity Framework then you could return an IQueryable and still take the advantange of a server query.

Why don't you use english variables names? Everything is in english but the parameters. This is not consistant... and it's difficult to get help with local names.

• germi: Could you add some arguments for why it's better to use StringComparer.[...] instead of .ToLower/.ToUpper? I'm not disagreeing, just thinking that it might be helpful.
• t3chb0t: @germi updated ;-)

Answer 2

I would do it differently.

1. U have too many parameter, use some model for this:

   public class FilterVM
   {
    public string Voornaam { get; set; }
    public string Achternaam { get; set; }
    public int? Language { get; set; }
    public int? Profiel { get; set; }
    public DateTime? Vanaf { get; set; }
    public DateTime? Tot { get; set; }
    public bool? Geslaagd { get; set; }
   }
   

   So yours method should be look like this:

   private IEnumerable<Registration> Filter(FilterVM filterVM)
   {
    return null
   }
   

2. Write some extension:

   public static IQueryable<TSource> WhereIf<TSource>(this IQueryable<TSource> source, bool condition, Expression<Func<TSource, bool>> predicate)
   {
    if (condition)
    return source.Where(predicate);
    return source;
   }
   

3. Use this extension in method:

   private IQueryable<Registration> Filter(FiterVM filter)
   {
    return All()
    .WhereIf(!string.IsNullOrEmpty(filter.Voornaam), x => x.FirstName.ToLower().StartsWith(filter.Voornaam.ToLower().Trim())
    .WhereIf(!string.IsNullOrEmpty(filter.Achternaam), x => x.LastName.ToLower().StartsWith(filter.Achternaam.ToLower().Trim())
    .WhereIf(language != null, x => x.LanguageId == filter.Language.Value)
    ...;
   }
   

   Shorted version:

   private IQueryable<Registration> Filter(FiterVM filter) => All()
    .WhereIf(!string.IsNullOrEmpty(filter.Voornaam), x => x.FirstName.ToLower().StartsWith(filter.Voornaam.ToLower().Trim())
    .WhereIf(!string.IsNullOrEmpty(filter.Achternaam), x => x.LastName.ToLower().StartsWith(filter.Achternaam.ToLower().Trim())
    .WhereIf(language != null, x => x.LanguageId == filter.Language.Value)
    ...;
   

• shelbypereira: This is my preference over the accepted answe, a really elegant and reusable extension method

Answer 3

Your filter steps all look like this:

if (...)
{
 all = [...].ToList();
}

That means that you're materialising all up to 7 times into a List<Registration>. If you'd omit the .ToList at the end, the evaluation would run only once and you would not have intermediate lists that you don't need. This is even more important in connection with what @t3chb0t said: If you get an IQueryable from your database, you can construct your query along the way and only execute it once you have all your conditions built.

Answer 4

Every time you write

all = (from r in all
where /*condition*/
select r).ToList();

You could simply use

all = all.Where(r => /*condition*/);

Reducing your code to:

IEnumerable<Registration> all = All(); // <-- this gets all registrations from 
 // the database.
if (!string.IsNullOrEmpty(voornaam))
{
 all = all.Where(r => r.FirstName.ToLower().StartsWith(voornaam.ToLower().Trim());
}
if (!string.IsNullOrEmpty(achternaam))
{
 all = all.Where(r => r.LastName.ToLower().StartsWith(achternaam.ToLower().Trim());
}
if (language != null)
{
 all = all.Where(r => r.LanguageId == language.Value);
}
if (profiel != null)
{
 all = all.Where(r => r.ProfileId == profiel.Value);
}
if (vanaf != null)
{
 all = all.Where(r => r.RegistrationDate >= vanaf.Value);
}
if (tot != null)
{
 all = all.Where(r => r.RegistrationDate <= tot.Value);
}
if (geslaagd != null)
{
 all = all.Where(r => r.Success == geslaagd.Value);
}
return all;