3
\$\begingroup\$

I think I've done a decent job keeping this query simple and understandable, but I'd like to know if you have some more advice.

My relevant entities are defined like this:

public class Task
{
    [ForeignKey("Employee")]
    public string EmployeeId { get; set; }
    public Employee Employee { get; set; }

    [Required]
    [ForeignKey("Status")]
    public int StatusId { get; set; }
    public TaskStatus Status { get; set; }

    [...]
}

public class Employee
{
    [Key]
    [DatabaseGenerated(DatabaseGeneratedOption.None)]
    [Required]
    [MaxLength(10)]
    [Index(IsUnique=true)]
    public string EmployeeId { get; set; }

    [Required]
    [StringLength(100)]
    public string FirstName { get; set; }

    [Required]
    [StringLength(100)]
    [DisplayName("Last name")]
    public string LastName { get; set; }

    [...]
}

public class TaskStatus
{
    [Key]
    public int Id { get; set; }

    [Required]
    [MaxLength(50)]
    public string Name { get; set; }

    [...]
}

I've stripped off what I believe are irrelevant parts of the entities.

The query itself is following:

// Retrieves all tasks from the database. We filter the tasks on 3 different criteria. Since all 3 criteria
// must be fulfilled for the task to be displayed, we use logical and to concatenate them.
// 
// The first criteria is the search term. If the search term is empty, we don't filter out any tasks, so 
// that is the first check we make. Also, we test if the FistName, LastName or the EmployeeId contain the 
// search term.
//
// Next criteria is the status filter. We keep the tasks that have the right status, or all tasks if the 
// given status is null.
//
// The last criteria is employee filter, and the logic is the same as for the status filter.
var tasks = db.Tasks
    .Include(t => t.Employee)
    .Include(t => t.Status)
    .Where(t =>
        (
            search == null
            || t.Employee.FirstName.Contains(search)
            || t.Employee.LastName.Contains(search)
            || t.Employee.EmployeeId.Contains(search)
        )
        && ((statusFilter == null) || (t.StatusId == statusFilter.Value))
        && ((employeeFilter == null) || (t.EmployeeId == employeeFilter))
    );

The query does what it is supposed to do, it filters the task list properly on all 3 parameters. I'm just worried that it is a little too unreadable (too tricky), especially since my colleagues are mostly young and inexperienced.

\$\endgroup\$
1
  • \$\begingroup\$ You could probably make the &&s into more Wheres. Otherwise, I doubt you can change it much. \$\endgroup\$ – Magus Apr 15 '14 at 15:04
2
\$\begingroup\$
  1. Remove the additional ()s in the statusFilter and employeeFilter lines as they are unnecessary.
  2. Indent the statusFilter and employeeFilter lines so that they are symmetrical with the search lines.
  3. Change search to be searchTerm as it is more specific. search by itself could mean anything like a stored query, a boolean value, etc.
  4. As per Magus' comment you can change t to task since it is more clear and will not add space that starts pushing statements across extra lines.
var tasks = db.Tasks
  .Include(task => task.Employee)
  .Include(task => task.Status)
  .Where(task =>
    (
      searchTerm == null || 
      task.Employee.FirstName.Contains(searchTerm) ||
      task.Employee.LastName.Contains(searchTerm) ||
      task.Employee.EmployeeId.Contains(searchTerm)
    ) &&
    (
      statusFilter == null || 
      task.StatusId = statusFilter.Value
    ) &&
    (
      employeeFilter == null || 
      task.EmployeeId == employeeFilter
    )
);
\$\endgroup\$
1
  • 1
    \$\begingroup\$ Agreed, though I'd also rename t to task or something. \$\endgroup\$ – Magus Apr 15 '14 at 15:17
2
\$\begingroup\$

I would actually build the linq query in stages. It will make the code more readable. Because execution is deferred until the list is actually iterated, performance is no different than chaining everything together in one long, obscure command.

var tasks = db.Tasks
    .Include(task => task.Employee)
    .Include(task => task.Status);

if ( searchTerm != null ) {
    tasks = tasks.Where(task =>
    (
        task.Employee.FirstName.Contains(searchTerm) ||
        task.Employee.LastName.Contains(searchTerm) ||
        task.Employee.EmployeeId.Contains(searchTerm)
    );
}

if ( statusFilter != null )
{ 
    tasks = tasks.Where( task => task.StatusId = statusFilter.Value );
}

if ( employeeFilter != null )
{
     tasks = tasks.Where( task => task.EmployeeId == employeeFilter);
}
\$\endgroup\$
1
  • \$\begingroup\$ Separating the null checks out into if statements is a much clearer way of expressing that you only need to apply each filter if it has a value. \$\endgroup\$ – skeletank Apr 16 '14 at 11:54

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.