Refactor two methods that generate a SelectList into a single method

Question

I have the following two methods that get data from my DB and return a populated SelectList object (including an "All" option value) that I then pass onto my view. The problem is that they are almost identical with the exception that they both access different repository objects and they have different ID names (StatusId and TeamId). I think there is an opportunity to refactor them into a single method that accepts the repository as a parameter and somehow figures out what the ID name should be, perhaps by using reflection or some sort of lambda expression, but I don't know quite how to accomplish this.

private SelectList GetStatusSelectList(int selectedStatusId)
{
  List<MemberStatus> statusList = _memberStatusRepository.All().ToList();
  statusList.Insert(0, new MemberStatus {StatusId = 0, Name = "All"});
  var statusSelectList = new SelectList(statusList, "StatusId", "Name", selectedStatusId);
  return statusSelectList;
}

private SelectList GetTeamSelectList(int selectedTeamId)
{
  List<MemberTeam> teamList = _memberTeamRepository.All().ToList();
  teamList.Insert(0, new MemberTeam { TeamId = 0, Name = "All" });
  var teamSelectList = new SelectList(teamList, "TeamId", "Name", selectedTeamId);
  return teamSelectList;
}

Can anyone help figure out how to refactor these into a single method?

Cassaundra

Are you able to edit those classes i.e. add an interface? – Ivoryivorywhite

Yes... both the _memberTeamRepostiory and _memberStatusRepository implement an IRepository interface. That interface has all the methods to interact with the DB including methods like IQueryable<TEntity> All() which simply returns the _dbSet of the given TEntity. – Cassaundra

how about MemberTeam and MemberStatus -- can you modify them directly or with partial classes? – Kook

Yes, I created them and they both can be edited, though both are concrete entities and do not (currently) implement any interfaces. – Cassaundra

Answer 1

you may try the following:

private SelectList GetStatusSelectList(int selectedStatusId)
{
    return GetGenericSelectList<MemberStatus>(selectedStatusId, _memberStatusRepository.All().ToList(), "StatusId");
}

private SelectList GetTeamSelectList(int selectedTeamId)
{
    return GetGenericSelectList<MemberTeam>(selectedTeamId, _memberTeamRepository.All().ToList(), "TeamId");
}

private SelectList GetGenericSelectList<T>(int selectedTeamId, List<T> list, string idFieldName) where T : new()
{
    var firstItem = new T();
    (firstItem as dynamic).Name = "All";
    var l = new List<T>(list);
    l.Insert(0, firstItem);
    return new SelectList(l, idFieldName, "Name", selectedTeamId);
}

This solution is not ideal and relies on some conventions (e.g. all your items should have Name property). However it seems to be a not bad way to start with. It may be improved further by using Expressions instead of property names -- that would allow to change property names with compile time check.

Loading

Thanks for the code. This seems to work, but I have two questions for you. First, I removed 3rd and 4th lines of your code and simply used 'list.Insert(0, firstItem)' instead. Is there any issues with this? Secondly, I don't know what the 'where T : new()' means in the method signature. Can you let me know what this is doing? – Cassaundra

I create a new list from argument, because otherwise existing list will be modified (you pass an existing list and function inserts a new element to it). That may not be an issue, but what if that original list is used elsewhere beside this method? Regarding new constraint -- it just allows to do new T(). See msdn.microsoft.com/en-us/library/sd2w2ew5.aspx for more details. – Loading

This is the cleanest solution to my immediate needs, so thank you the_joric! – Cassaundra

Answer 2

Well, this is the most generic I can come up with, but it will require that your MemberStatus and MemberTeam implement IIdentifiable, which I don't know if can apply to your case. If so, this would be the way to go.

private SelectList GetList<T>(IRepository repository, int id, string name)
    where T : IIdentifiable, new()
{
    List<IIdentifiable> list = repository.All().ToList();
    list.Insert(0, new T() { Name = name, Id = id });
    var statusSelectList = new SelectList(list, "Id", "Name", id);
}

And the interface code

interface IIdentifiable
{
    int Id { get; set; }
    string Name { get; set; }
}

Ivoryivorywhite

Thanks Tomislav. One concern though, my POCO objects for MemberTeam and MemberStatus are very simple and both have a Name property, but each has a unique name for the Id property (StatusId and TeamId). Is there a way to keep these properties named as is and still implement the interface you describe? I tend to like having the name more descriptive for the DB design, but could be persuaded to use a more generic field of just Id if it's recommended. – Cassaundra

Of course, just map the IIdentifiable.Id property to StatusId and TeanId in your classes. int Id { get { return StatusId; } set { StautsId = value; } } in your MemberStatus class. Wouldn't this work? – Ivoryivorywhite

@Tomislav... thank you for this. As I mentioned to foson, who posted something similar, I am going to use your method for a more robust refactoring of my domain model, but the_joric's answer is the most concise for the question I posted, so I'm going to accept his post. But again, thank you for the input and you showed me some new directions to head as I continue to build out this project! – Cassaundra

Answer 3

From what I see, the main hurdles in the way of refactoring this into a single method are the new MemberStatus and the new MemberTeam calls, in addition to determining the right repository to use.

To come up with an elegant solution, you would need a little more infrastructure configured - basically you would need to resolve the correct repository based on the type and you would want to have some sort of factory build up the object instance.

The following will refactor the code into a single method, but isn't (in my opinion) any better than the separate methods you already have:

private SelectList GetSelectList<T>(int selectedId, Func<List<T>> repoAllFunc, Func<T> typeNewFunc, string idName)
{
    List<T> list = repoAllFunc();
    list.Insert(0, typeNewFunc());
    var selectList = new SelectList(list, idName, "Name", selectedId);
    return selectList;
}

You could then call it like this:

var memberStatusSelectList = 
    GetSelectList<MemberStatus>(
        id, 
        () => _memberStatusRepository.All().ToList(),
        () => new MemberStatus {StatusId = 0, Name = "All"});

Idiot

Ethan, I'm trying your code out as well, but having a tough time figuring out how to call your method. I assume that it takes a lambda expression, but since I'm new to these, can you give me a pointer as to what I pass in for the Func<> parameters? – Cassaundra

@bmccleary I added an example of how to use. The funcs in this example take no parameters so the syntax is as simple as it gets for the delegate syntax. Otherwise, they get a little uglier. – Idiot

thank you for the sample code and explanation. I am going to accept the_joric's answer for now as it's a little cleaner for my immediate needs, but I have been trying to figure out how to pass delegates into methods for a while, and your sample provides me some direction on how to do this for other areas of my code, so thank you very much! – Cassaundra

Answer 4

You can go a little interface crazy and do the following:

using System;
using System.Collections.Generic;
using System.Linq;

namespace ConsoleApplication3
{

    public class MemberStatus : IDefault<MemberStatus>
    {
        public int StatusId { get; set; }
        public string Name { get; set; }

        public MemberStatus Default
        {
            get { return new MemberStatus() { StatusId = 0, Name = "All" }; }
        }

        public string IdName
        {
            get { return "StatusId"; }
        }
    }

    public class MemberTeam : IDefault<MemberTeam>
    {
        public int TeamId { get; set; }
        public string Name { get; set; }

        public MemberTeam Default
        {
            get { return new MemberTeam() { TeamId = 0, Name = "All" }; }
        }

        public string IdName
        {
            get { return "TeamId"; }
        }
    }

    public interface IDefault<T>
    {
        T Default { get; }
        string IdName { get; }
    }

    public interface IRepository<T>
    {
        IEnumerable<T> All();
    }

    public class MemberStatusRepository : IRepository<MemberStatus>
    {
        public IEnumerable<MemberStatus> All()
        {
            return new[] { 
                new MemberStatus(),
                new MemberStatus()
            };
        }
    }
    public class MemberTeamRepository : IRepository<MemberTeam>
    {
        public IEnumerable<MemberTeam> All()
        {
            return new[] { 
                new MemberTeam(),
                new MemberTeam()
            };
        }
    }

    public class DataAccessLayer
    {
        IRepository<MemberStatus> _memberStatusRepository;
        IRepository<MemberTeam> _memberTeamRepository;
        public DataAccessLayer()
        {
            _memberStatusRepository = new MemberStatusRepository();
            _memberTeamRepository = new MemberTeamRepository();
        }


        public SelectList<TResult> GetTeamSelectList<TRepository, TResult>(TRepository repo, int selectedTeamId)
            where TRepository : IRepository<TResult>
            where TResult : IDefault<TResult>, new()
        {
            List<TResult> teamList = repo.All().ToList();
            var dummyobj = new TResult();
            teamList.Insert(0, dummyobj.Default);
            var teamSelectList = new SelectList<TResult>(teamList, dummyobj.IdName, "Name", selectedTeamId);
            return teamSelectList;
        }
    }

    class Program
    {
        static void Main(string[] args)
        {
            var dal = new DataAccessLayer();
            SelectList<MemberStatus> results = dal.GetTeamSelectList<IRepository<MemberStatus>, MemberStatus>(new MemberStatusRepository(), 5);
            Console.WriteLine();
            Console.Read();
        }
    }

    public class SelectList<TResult>
    {
        public SelectList(List<TResult> teamList, string p, string p_2, int selectedTeamId)
        {

        }
    }

}

It would be nice if you could define static properties in an interface, but since you can't I rely on creating a dummy object instead.

Kook

@foson... WOW! Thank you for all the code. I can see where you are heading here, and I think I might work on doing a little refactoring of my domain class to adopt your methodology, but for now, the_joric's answer was the most basic way to address my immediate needs, so I'm accepting it. But again, I'm keeping your code as a reference so that I can work on some more heavy refactoring in the near future, so thank you very much for your time and detail! – Cassaundra

NP. Like I said, my solution is a little interface crazy -- definitely more complicated/less readable than using dynamic. If I were using dynamic or reflection, personally I would be sure to do some perf testing to ensure the solution is within my acceptable perf expectations. – Kook

Answer 5

If IRepository adds a few "features," you'll get some cleaner code.

Instead of All() have a SingleRecordsWithAllRecord() method that handles the first two lines. Then have the repository define its own DataValueField and DataTextField.

private SelectList GetSelectList(IRepository repo, int selectedId)
{
   var selectListAll = repo.SingleRecordsWithAllRecord().ToList();

   return new SelectList(selectListAll, 
                         repo.DataValueField, 
                         repo.DataTextField, 
                         selectedId);
}

Amaras

Austin, I like your thought pattern here, and I think I will try it as I continue with the code, but for now, the_joric's answer was most direct to my immediate needs. Thanks for the input! – Cassaundra