Nested LINQ Nightmare, calculating %completed

Question

This code horrifies and scares me. I wrote this.

public IQueryable<StudentTrack> StudentTracks(int employeeId)
{

    IQueryable<StudentTrack> EducationPortalStudentTracks =
        from studentTrack in _dbContext.EducationPortalStudentTracks
        join track in _dbContext.EducationPortalTracks on studentTrack.TrackId equals track.Id
        orderby track.Id
        where studentTrack.EmployeeId == employeeId
        select new StudentTrack
        {
            Id = studentTrack.Id,
            TrackId = studentTrack.TrackId,
            Name = track.Name,
            Deadline = studentTrack.Deadline,

            // this is where it gets bad
            PercentComplete = (from doesntMatter in _dbContext.EducationPortalStudentCourses
                               let courses = (from studentCourse in _dbContext.EducationPortalStudentCourses
                                              join studentTrack_ in _dbContext.EducationPortalStudentTracks on studentCourse.TrackId equals studentTrack_.TrackId
                                              where studentTrack.Id == studentTrack.Id && studentTrack.EmployeeId == studentCourse.EmployeeId
                                              select studentCourse)
                               select courses.Where(x => x.Status == true).Count() * 100 / courses.Count()).FirstOrDefault()

        };
    return EducationPortalStudentTracks;
}

Let me explain how this fits in to the rest of the application. On the top level, a Kendo Grid control calls an MVC controller for its data. That controller calls another controller, which calls this. So this method returns the data which populates a grid in the UI. That grid lists all of the StudentTracks in the database. In order to return the StudentTracks, this method does a little bit of Linq and returns a series of StudentTrack objects.

So the problem starts with the PercentComplete field. To calculate the value for that field, we first find all of the children StudentCourses. We then figure out how many of the StudentCourses are complete (a.k.a. have True in the Status column) and divide that by the total number of StudentCourses.

Anyway, this is what I came up with. I'm not pleased with it, but I don't know how to do it any better. Any ideas on how I can make this a little more elegant?

Answer 1

Break it down into more methods. StudentTracks has multiple responsibilities, you could extract a private method whose job would be to CalculatePercentCompletion(studentTrack), making your method look like this:

public IQueryable<StudentTrack> StudentTracks(int employeeId)
{
    IQueryable<StudentTrack> EducationPortalStudentTracks =
        from studentTrack in _dbContext.EducationPortalStudentTracks
        join track in _dbContext.EducationPortalTracks on studentTrack.TrackId equals track.Id
        orderby track.Id
        where studentTrack.EmployeeId == employeeId
        select new StudentTrack
        {
            Id = studentTrack.Id,
            TrackId = studentTrack.TrackId,
            Name = track.Name,
            Deadline = studentTrack.Deadline,
            PercentComplete = CalculatePercentCompletion(studentTrack)    
        };

    return EducationPortalStudentTracks;
}

Then CalculatePercentCompletion could look like this:

private int CalculatePercentCompletion(StudentTrack studentTrack)
{
    return (from doesntMatter in _dbContext.EducationPortalStudentCourses
            let courses = (from studentCourse in _dbContext.EducationPortalStudentCourses
                           join studentTrack_ in _dbContext.EducationPortalStudentTracks on studentCourse.TrackId equals studentTrack_.TrackId
                           where studentTrack.Id == studentTrack.Id && studentTrack.EmployeeId == studentCourse.EmployeeId
                           select studentCourse)
            select courses.Where(x => x.Status == true).Count() * 100 / courses.Count()).FirstOrDefault();
}

...Which is still horrible.

Why are you using the query syntax? If you have navigation properties on your entities (assuming EF), I find this would be much easier to parse:

public IQueryable<StudentTrack> StudentTracks(int employeeId)
{
    return _dbContext.EducationPortalStudentTracks
                     .Where(studentTrack => studentTrack.EmployeeId == employeeId)
                     .OrderBy(studentTrack => studentTrack.Track.Id)
                     .Select(studentTrack => 
                         new StudentTrack
                         {
                             Id = studentTrack.Id,
                             TrackId = studentTrack.TrackId,
                             Name = studentTrack.Track.Name,  
                             Deadline = studentTrack.Deadline,
                             PercentComplete = CalculatePercentCompletion(studentTrack)
                         });
}

In CalculatePercentCompletion the doesntMatter part makes it pretty confusing, I'm not sure but I think this could be equivalent:

private int CalculatePercentCompletion(StudentTrack studentTrack)
{
    var courses = _dbContext.EducationPortalStudentCourses
                            .Where(course => course.TrackId == studentTrack.Id
                                          && course.EmployeeId == studentTrack.EmployeeId)
                            .ToList();
    return courses.Count(course => course.Status == true) * 100 / courses.Count
}

Why the .ToList() call? You want to materialize your query only when you need to - this method needs to know what the courses are for a given studentTrack, in order to count the number of courses with Status == true, multiply that by 100 and divide by the total number of courses.

Your code iterates courses twice, in the two Count() calls.

And the intent is not exactly clear because of the doesntMatter in the from clause, and the .FirstOrDefault() call makes it look like we're looking at an IEnumerable<int> when we're really all materialized at that point and we either have zero or one values selected (if I get this correctly).

The status == true part is confusing. Normally I would have written it as .Where(course => course.Status) to avoid comparing a Boolean with a Boolean value, but Status is really a poor name and doing that would have made it harder to understand what's going on. I suggest you rename Status to whatever IsCompleted, so the where condition can read as .Where(course => course.IsCompleted), which is crystal-clear.

---

I think exposing IQueryable<T> to your UI is a bad idea. StudentTracks should return an IEnumerable<StudentTrack> instead, your grid should be able to work off that just as well.

I would leave the last Select to Linq-to-Objects, and materialize the query just after the OrderBy call:

public IEnumerable<StudentTrack> GetStudentTracks(int employeeId)
{
    return _dbContext.EducationPortalStudentTracks
                     .Where(studentTrack => studentTrack.EmployeeId == employeeId)
                     .OrderBy(studentTrack => studentTrack.Track.Id)
                     .ToList()
                     .Select(studentTrack => 
                         new StudentTrack
                         {
                             Id = studentTrack.Id,
                             TrackId = studentTrack.TrackId,
                             Name = studentTrack.Track.Name,  
                             Deadline = studentTrack.Deadline,
                             PercentComplete = CalculatePercentCompletion(studentTrack)
                         });
}

The problem with exposing IQueryable<T> to your client code, is that you allow that code to modify the SQL being generated, and potentially to break your query with calls that Linq-to-Entities cannot translate to SQL. The grid in your UI doesn't need an IQueryable<T>.