Code redundancy issue while feeding dropdownlists

I have a dashboard to manage a learning center system. There are scenarios like admin create learning center and define a classroom in it and then assign some courses in it and assign students to those courses and assign a teacher for each courses.

In order to achieve this, I have to create lot of dropdownlists such as for creating a course you have to select first a learning center then an appropriate classroom.

Below is the code for the dropdownlists in assigning a student into a course:

```
//Retrieve all learning centers
studentAssignmentViewModel.LearningCenters = _learningCenterService.LearningCenters.ToList();
studentAssignmentViewModel.StudentSelectListItem = studentAssignmentViewModel.LearningCenters
.Where(y => y.Status != Status.Removed).OrderBy(x => x.Name)
.Select(x => new SelectListItem { Text = x.Name, Value = x.Id.ToString() });

//Retrieve All Students
studentAssignmentViewModel.Students = _studentAssignmentService.Students.ToList();
studentAssignmentViewModel.StudentSelectListItem = studentAssignmentViewModel.Students
.Where(y => y.Status != Status.Removed).OrderBy(x => x.Name)
.Select(x => new SelectListItem { Text = x.Name, Value = x.Id.ToString() });

//If the user is not admin, it retrieves the courses belong to the teacher otherwise all courses
var userId = _dashboardUserAuthenticationService.AuthenticatedDashboardUser.Id;
var userRole = _dashboardUserService.Get(userId).UserRole;
if (userRole != "1")
{
studentAssignmentViewModel.Courses = _studentAssignmentService.Courses.ToList();
studentAssignmentViewModel.CourseSelectListItem = studentAssignmentViewModel.Courses
.Where(t => (t.Status != Status.Removed) && t.TeacherId == userId).OrderBy(x => x.Name)
.Select(x => new SelectListItem { Text = x.Name, Value = x.Id.ToString() });
}
else
{
studentAssignmentViewModel.Courses = _studentAssignmentService.Co

It's a small thing, but you could move the bulk of the code for course selection outside of the if statement by initializing the list and then filtering it further only if the user is not an admin.

studentAssignmentViewModel.Courses = _studentAssignmentService.Courses.ToList();
    studentAssignmentViewModel.CourseSelectListItem = studentAssignmentViewModel.Courses
        .Where(t => (t.Status != Status.Removed)).OrderBy(x => x.Name)
        .Select(x => new SelectListItem { Text = x.Name, Value = x.Id.ToString() });        

    //If the user is not admin, it filter the course list further
    var userId = _dashboardUserAuthenticationService.AuthenticatedDashboardUser.Id;
    var userRole = _dashboardUserService.Get(userId).UserRole;
    if (userRole != "1")
    {
        studentAssignmentViewModel.CourseSelectListItem = studentAssignmentViewModel.CourseSelectListItem
            .Where(t => (t.TeacherId == userId).OrderBy(x => x.Name)
            .Select(x => new SelectListItem { Text = x.Name, Value = x.Id.ToString() });
    }

That may perform slightly worse because now it has to iterate twice if the user isn't an admin, but you're no longer repeating yourself. Any change to the logic would only have to occur in one place.

Now there's this...

if (userRole != "1")

If you changed this to use an enum instead, that comment becomes obsolete and can be removed. Always try to let the code document itself. I would much rather see this.

if (userRole != UserRole.Admin)

Also, you say you're repeating this logic elsewhere in your code. You've not shown it, but I highly recommend extracting a new class that is responsible for this logic. You can then inject it into these other classes via their constructor and any changes to this logic truly only has to occur in one place.

StackExchange Code Review Q#73537, answer score: 4