Code redundancy issue while feeding dropdownlists

Question

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.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() });
    }

And I am repeating those similar code pieces in edit and create actions in mvc for all scenarios. So I don't know if there is enough information to make a statement but what would be your suggestions for such cases, what would be the best practice to get rid of those ugly code pieces? Since situations vary, turning them into classes would lead me into so many specific classes which cannot be used for any other situation.

Answer 1

This calls for an interface! If you create an interface with the properties you're duplicating and have all of your classes implement the interface, you can create either a method that takes in an IEnumerable<IInterface> and returns a new, filtered IEnumerable<IInterface>, or a delegate method that you can call on each item in the collection, calling yield return item; if it meets your criteria or yield break; if it doesn't. Your app seems to be well-enough designed that you won't run into any casting issues, either.

Most straightforward example:

public interface ISelectListBindable
{
    int Id { get; set; }
    string Name { get; set; }
}

public IEnumerable<T> GetSelectListItemsForEntities(IEnumerable<T> entities) where T : ISelectListBindable
{
    return entities.OrderBy(x => x.Name)
        .Select(x => new SelectListItem { Text = x.Name, Value = x.Id.ToString() });
}

I used a type constraint instead of making the input parameter IEnumerable<ISelectListBindable> so you can maintain the current type you're using if you need to instead of having to upcast/downcast.

I took out the Status.Removed logic in this method even though it's duplicated. You should probably encapsulate your filtering for checking Removed within an object that only returns objects that aren't removed up to the presentation layer.

With this interface approach you can let your app naturally evolve as you break out methods and rearrange them into classes and layers (as opposed to establishing a line of demarcation up front and attempting to confine your code to respective areas). This way will lead to a distinct delineation between your presentation-oriented objects and your business objects.

Answer 2

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.

Answer 3

There is too much logic going on here, I think if you break things out more it will reduce the complexity and make your code more modular.

So for the learning centers it might look something like this:

//Retrieve all learning centers
studentAssignmentViewModel.LearningCenters = _learningCenterService.LearningCenters.ToList();

studentAssignmentViewModel.StudentSelectListItem 
        = _learningCenterService.GetNotRemovedLearningCenters()
                                .Select(MapLearningCenterToSelectListItem);

public class LearningCenterViewModelMapper
{
    public SelectListItem MapLearningCenterToSelectListItem(LearningCenter learningCenter)
    {
        return new SelectListItem { Text = learningCenter.Name, Value = learningCenter.Id.ToString() };
    }
}

public class LearningCenterService
{
    public IEnumerable<LearningCenter> GetNotRemovedLearningCenters()
    {
        return _learningCenters.Where(y => y.Status != Status.Removed).OrderBy(x => x.Name);
    }
}

Please not I wrote this code by hand, made some assumptions about your code structure, and did not test it. This is just to give you a general idea.

Here are some other points:

• Logic wise, it's strange to set the value of LearningCenters then set the SelectListItem based on that, it should be set against the service too.
• if(userRole != "1"). This is a magic number, consider refactoring.
• Again for the Admin section I would refactor and put that logic into the service. You can also use the mapping code for the mapping to SelectListItem.