0
\$\begingroup\$

I need to filter my records on the basis of "courseId" which is passed as parameter -
Here is my code -

public IEnumerable<string> GetStudentIdsByRoleAndCourse(string role, string courseId = null)
{
  var query = (from item in Data.TableForQuery<MasterTable>()
               where item.RoleId == role
               select item).ToList();

  if (courseId != null)
  {
    return (from item in query
            let student = Students.GetModelById(item.StudentId)
            where student.DisplayInReports && student.CourseId == courseId 
            orderby student.Name
            select student.Id).ToList();
  }

  return (from item in query
          let student = Students.GetModelById(item.StudentId)
          where student.DisplayInReports 
          orderby student.Name
          select student.Id).ToList();
}

Because I was not convinced with writing separate return statements -
I have now modified the code as below - to cater for the nullable parameter courseId

public IEnumerable<string> GetStudentIdsByRoleAndCourse(string role, string courseId = null)
{
  var query = (from item in Data.TableForQuery<MasterTable>()
               where item.RoleId == role
               select item).ToList();
  
    return (from item in query
            let student = Students.GetModelById(item.StudentId)
            where student.DisplayInReports 
            && (courseId == null || student.CourseId == courseId)
            orderby student.Name
            select student.Id).ToList();        
} 

Wondering if there is even better way than above to achieve same.

Thanks a lot!

\$\endgroup\$
4
  • 4
    \$\begingroup\$ wither courseId has a value or is null, in both cases && student.CourseId == courseId will cover it. So, the extra courseId == null || is not needed. You can verify that yourself. Also, you should consider using join to join MasterTable with the Students to get the results in one query, this would saves the round-trip, and reduces the memory allocation (it will only store the results in the memory). \$\endgroup\$
    – iSR5
    Commented Feb 8, 2022 at 18:58
  • \$\begingroup\$ @iSR5, thank you for your reply and feedback on join between MasterTable and Students. Students is in cache here - so there is only one call to database. On the other side, I tested removing courseId == null but it doesn't work - instead of returning all the students it doesn't return any records in that case. Of course, when courseId is not null, in that case it correctly filters Students for courseId. \$\endgroup\$
    – iniki
    Commented Feb 8, 2022 at 22:14
  • \$\begingroup\$ If there is no records, then student.CourseId is not nullable. In that case, you could revise it to student.CourseId == courseId ?? student.CourseId. \$\endgroup\$
    – iSR5
    Commented Feb 8, 2022 at 22:30
  • \$\begingroup\$ Your design is bad. courseId and RoleId and student.Id shouldn't be strings, they should be ints or guids. \$\endgroup\$
    – BCdotWEB
    Commented Feb 9, 2022 at 7:49

1 Answer 1

1
\$\begingroup\$

Query syntax for Linq here makes this harder than it should be. I'd recommend using Linq's method syntax because it can be built as pieces.

public IEnumerable<string> GetStudentIdsByRoleAndCourse(string role, string courseId = null)
{
    var filteredQuery = Data.TableForQuery<MasterTable>()
                            .Where(_ => _.RoleId == role)
                            .Select(_ => Students.GetModelById(_.StudentId))
                            .Where(_ => _.DisplayInReports);
    if(courseId != null)
    {
        filteredQuery = filteredQuery.Where(_ => _.CourseId == courseId);
    }
    return filteredQuery.OrderBy(_ => _.Name)
                        .Select(_ => _.Id)
                        /*ToList is completely optional since your method is only 
                          IEnumerable<string>, calling ToList every where is
                          actually a bad thing*/
                        .ToList();
  
}
\$\endgroup\$

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service and acknowledge you have read our privacy policy.

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