2
\$\begingroup\$

mappingService below is actually just Automapper. I have a wrapper around it so that I can inject it.

I want to refactor the following code to be cleaner:

 public class DropDownListService : IDropDownListService
{
    private readonly IMappingService _mappingService;
    private readonly ICompanyRepository _companyRepository;
    private readonly IProjectTypeRepository _projectTypeRepository;
    private readonly IStatusRepository _statusRepository;
    private readonly IProjectManagerRepository _projectManagerRepository;
    private readonly IProjectArchitectRepository _projectArchitectRepository;
    private readonly IFeeTypeRepository _feeTypeRepository;
    private readonly IStateRepository _stateRepository;
    private readonly IPersonTypeRepository _personTypeRepository;

    public DropDownListService(IMappingService mappingService, ICompanyRepository companyRepository, IProjectTypeRepository projectTypeRepository, IStatusRepository statusRepository, IProjectManagerRepository projectManagerRepository, IProjectArchitectRepository projectArchitectRepository, IFeeTypeRepository feeTypeRepository, IStateRepository stateRepository, IPersonTypeRepository personTypeRepository)
    {
        _mappingService = mappingService;
        _companyRepository = companyRepository;
        _projectTypeRepository = projectTypeRepository;
        _statusRepository = statusRepository;
        _projectManagerRepository = projectManagerRepository;
        _projectArchitectRepository = projectArchitectRepository;
        _feeTypeRepository = feeTypeRepository;
        _stateRepository = stateRepository;
        _personTypeRepository = personTypeRepository;
    }

    public IEnumerable<IDropDownList> RetrieveCompanies()
    {
        return _mappingService.Map(_companyRepository.GetAll(), new List<Company>());
    }

    public IEnumerable<IDropDownList> RetrieveJobTypes()
    {
        return _mappingService.Map(_projectTypeRepository.GetAll(), new List<ProjectType>());
    }

    public IEnumerable<IDropDownList> RetrieveStatuses()
    {
        return _mappingService.Map(_statusRepository.GetAll(), new List<Status>());
    }

    public IEnumerable<IDropDownList> RetrieveProjectManagers()
    {
        return _mappingService.Map(_projectManagerRepository.GetAll(), new List<ProjectManager>());
    }

    public IEnumerable<IDropDownList> RetrieveProjectArchitects()
    {
        return _mappingService.Map(_projectArchitectRepository.GetAll(), new List<ProjectArchitect>());
    }

    public IEnumerable<IDropDownList> RetrieveFeeTypes()
    {
        return _mappingService.Map(_feeTypeRepository.GetAll(), new List<FeeType>());
    }

    public IEnumerable<IDropDownList> RetrievePhases()
    {
        return _mappingService.Map(_feeTypeRepository.GetAll(), new List<Phase>());
    }

    public IEnumerable<IDropDownList> RetrieveStates()
    {
        return _mappingService.Map(_stateRepository.GetAll(), new List<State>());
    }

    public IEnumerable<IDropDownList> RetrievePeopleTypes()
    {
        return _mappingService.Map(_personTypeRepository.GetAll(), new List<PersonType>());
    }

    public IEnumerable<IDropDownList> RetrieveMarkets()
    {
        return _mappingService.Map(_marketRepostiory.GetAll(), new List<Market>());
    }
}

As you can see I am injecting way too many things into this controller. I am not currently doing a spa application so I cannot do this separately on the presentation layer. What I would really like to do is use generics. The problem is, the DB value that is returned to me looks like this:

    public int StateID { get; set; }
    public string StateName { get; set; }

Each entity has the ID field and the name field only they are named for the table. I am unable to write one map (as far as I know how) with automapper to map them to the correct type.

How can I clean this class up?

\$\endgroup\$
3
  • \$\begingroup\$ The title of your post should be the function/purpose of your code. \$\endgroup\$
    – SirPython
    Mar 21 '15 at 14:36
  • \$\begingroup\$ Also, you should list what language this is, that's your most important tag. \$\endgroup\$
    – Legato
    Mar 21 '15 at 14:43
  • \$\begingroup\$ @SirPython Fixed that, sorry haven't posted on this site in a while and should have re-read the rules for posting. \$\endgroup\$
    – Robert
    Mar 21 '15 at 15:01
1
\$\begingroup\$

I may have gotten your question wrong... Will the following implementation work for you? Can you show how your IMappingService looks like?

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

public interface ICompanyRepository : IRepository<Company> { }

public class DropDownListService<T> : IDropDownListService
{
    private readonly IMappingService _mappingService;
    private readonly IRepository<T> _repository;

    public DropDownListService(IMappingService mappingService, IRepository<T> repository)
    {
        _mappingService = mappingService;
        _repository = repository;
    }

    public IEnumerable<IDropDownList> RetrieveDropDownList()
    {
        return _mappingService.Map(_repository.GetAll(), new List<T>());
    }
}
\$\endgroup\$
2
  • \$\begingroup\$ Is it possible to pass IRepository<T> to a constructor. I thought you had to specify the type in order to inject it \$\endgroup\$
    – Robert
    Mar 22 '15 at 15:15
  • \$\begingroup\$ It depends on how you register it in your DI container. With Autofac you can register a generic DropDownListService<T> with the dependency on IRepository<T> like this: builder.RegisterGeneric(typeof (IEnumerable<>)).AsImplementedInterfaces();, and it will resolve proper IRepository<T> \$\endgroup\$
    – almaz
    Mar 23 '15 at 10:42
1
\$\begingroup\$

The number of dependencies suggests this class is doing too much work and violates single responsibility principle. Instead of having one class factor all different enumerations, have multiple classes, e.g. one for project members, one for states, etc.

\$\endgroup\$
3
  • \$\begingroup\$ I considered that, except then my MVC Controller will have the same number of injections. I was hoping for a clean solution that would allow me to inject one class and decide from there \$\endgroup\$
    – Robert
    Mar 21 '15 at 19:34
  • \$\begingroup\$ Please provide an example along with your point/opinion to add context to your post, giving the author of the original post (and everyone else that reads this answer) a better grasp at what you are saying. \$\endgroup\$
    – SirPython
    Mar 21 '15 at 20:37
  • \$\begingroup\$ @Robert, that wouldn't change the underlying problem. I'd argue such a solution would be easy, but not clean (think unit tests). However you could still combine similar services into smaller facades (e.g. a TypeService, PersonService,...) and let your master class bundle them. At least you can reduce the number of dependencies per class then. \$\endgroup\$
    – sunside
    Mar 22 '15 at 12:35

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service, privacy policy and cookie policy

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