Asp.net - Refactor action

Question

Hi!

I have this "problem" that i have code like this in many of my controller actions:

var users = new List<SelectListItem>();
foreach(var user in userRepository.GetAll())
{
 var item = new SelectListItem { Text = user.FriendlyName, Value = user.UserId.ToString() };

 if (User.Identity.Name == user.UserName)
  item.Selected = true;

 users.Add(item);
}

ViewData["Users"] = users;

How would you refactor this to a more clean solution? I want to get DRY!

Answer 1

You could place that logic in an (abstract) BaseController and then derive all your controllers from it and have them call a method in the BaseController to get that data as needed.

If you need to include it in ALL your controller actions you could also place it in an overridden OnActionExecuting() in the BaseController.

Answer 2

I would create this to be an extension method applied to List<user> or whatever your userRepository.GetAll() returns so then in your code you can replace all of these usages with just

ViewData["Users"] = userRepository.GetAll().ToSelectList();

Edit code sample: There's 2 ways you could do this

public static List<SelectListItem> ToSelectList(this List<Agent> users)
{
    List<SelectListItem> items = new List<int>();
    foreach (var user in users)
    {
        var item = new SelectListItem { Text = user.FriendlyName, 
                                Value = user.UserId.ToString() };

        if (User.Identity.Name == user.UserName)
            item.Selected = true;

        items.Add(item);
    }

    return items;
}

And usage would be like

userRepository.GetAll().ToSelectList();

Or if you have problems with identity being in the extension method have

public static List<SelectListItem> ToSelectList(this List<Agent> users, 
                                                        string selectedUserName)
{
    List<SelectListItem> items = new List<int>();
    foreach (var user in users)
    {
        var item = new SelectListItem { Text = user.FriendlyName, 
                                Value = user.UserId.ToString() };

        if (user.UserName == selectedUserName)
            item.Selected = true;

        items.Add(item);
    }

    return items;
}

And usage would be like

userRepository.GetAll().ToSelectList(User.Identity.Name);

Answer 3

Put that code, or part of the code into a separate class - let's call it UserService. Let UserService implement IUserService:

public interface IUserService
{
    IEnumerable<SelectListItem> GetUsers();
}

Inject IUserService into your controller via Constructor Injection:

public MyController(IUserService userService)
{
    this.userService = userService;
}

Use the userService field in your Controller Actions to get the users.

public ViewResult DoSomething()
{
    var users = this.userService.GetUsers();
    // the rest of the implementation
}