5
\$\begingroup\$

I'm working on a fairly mundane ASP.NET MVC app that allows users to do basic CRUD operations on a number of different domain objects. I'm trying to minimize duplicate code and boring, monkey code as much as possible, so I'm using AutoMapper to map types and MVC's scaffolding feature to spit out forms. I have a standard template for Index, Details, Create, Edit, Delete, and DeleteConfirmed views. Finally, I use specific view models to support these views (IndexModel, RowModel, DisplayModel, and EditModel).

I've written an abstract base class called StandardService, which works with AutoMapper to provide support for mapping from entities to view models and back again. It has generic type parameters for the entity and each type of view model.

I began a refactoring of this class based on concern that it violated SRP (and had "too many" generic parameters), but I quickly realized this just meant a lot more classes and a lot more code. Yes, the classes would be more tightly focused, and there would no doubt be greater flexibility, but it didn't seem to be flexibility I needed at this point.

So, I reverted to the single base class. I'd like feedback on whether this is a sensible/practical use of inheritance or an anti-pattern/code smell. Is there a better solution that doesn't result in an explosion of classes?

StandardService`5

public class StandardService<TEntity, TIndexModel, TRowModel, TDisplayModel, TEditorModel> : ServiceBase
    where TEntity : class, IEntity
    where TIndexModel : IIndexModel<TRowModel>
    where TDisplayModel : IDisplayModel
    where TEditorModel : IEditorModel, new()
{
    public StandardService(MainDbContext dbContext) : base(dbContext) { }

    public virtual TIndexModel GetIndexModel(GridSortOptions sortOptions)
    {
        var processedEntities = GetProcessedEntities();
        var rowModels = MapProcessedEntitiesToRowModels(processedEntities);
        var processedRowModels = ProcessRowModels(rowModels);
        processedRowModels = SortRowModels(processedRowModels, sortOptions);
        var model = MapProcessedRowModelsToIndexModel(processedRowModels);
        model.SortOptions = sortOptions;
        return model;
    }

    protected virtual IEnumerable<TEntity> GetProcessedEntities()
    {
        return GetDbSet();
    }

    protected virtual IEnumerable<TRowModel> MapProcessedEntitiesToRowModels(IEnumerable<TEntity> processedEntities)
    {
        return Mapper.Map<IEnumerable<TRowModel>>(processedEntities);
    }

    protected virtual IEnumerable<TRowModel> ProcessRowModels(IEnumerable<TRowModel> rowModels)
    {
        return rowModels;
    }

    protected virtual IEnumerable<TRowModel> SortRowModels(IEnumerable<TRowModel> rowModels, GridSortOptions sortOptions)
    {
        if (sortOptions == null || string.IsNullOrWhiteSpace(sortOptions.Column)) return rowModels;
        return rowModels.OrderBy(sortOptions.Column, sortOptions.Direction);
    }

    protected virtual TIndexModel MapProcessedRowModelsToIndexModel(IEnumerable<TRowModel> processedRowModels)
    {
        return Mapper.Map<TIndexModel>(processedRowModels);
    }

    public virtual TDisplayModel GetDisplayModel(int id)
    {
        var entity = FindOrThrow(id);
        return MapEntityToDisplayModel(entity);
    }

    protected virtual TDisplayModel MapEntityToDisplayModel(TEntity entity)
    {
        return Mapper.Map<TDisplayModel>(entity);
    }

    public virtual TEditorModel GetEditorModel()
    {
        return new TEditorModel();
    }

    public virtual TEditorModel GetEditorModel(int id)
    {
        var entity = FindOrThrow(id);
        return MapEntityToEditorModel(entity);
    }

    public virtual void UpdateModel(TEditorModel model) { }

    public virtual TEntity AddAndSave(TEditorModel model)
    {
        var entity = MapEditorModelToEntity(model);
        GetDbSet().Add(entity);
        SaveChanges();
        return entity;
    }

    protected virtual TEntity MapEditorModelToEntity(TEditorModel model)
    {
        return Mapper.Map<TEntity>(model);
    }

    public virtual TEntity EditAndSave(TEditorModel model)
    {
        var entity = FindOrThrow(model.Id);
        MapEditorModelToEntity(model, entity);
        SaveChanges();
        return entity;
    }

    protected virtual TEditorModel MapEntityToEditorModel(TEntity entity)
    {
        return Mapper.Map<TEditorModel>(entity);
    }

    protected virtual void MapEditorModelToEntity(TEditorModel model, TEntity entity)
    {
        Mapper.Map(model, entity);
    }

    public virtual bool CanDelete(int id)
    {
        var entity = FindOrThrow(id);
        return CanDelete(entity);
    }

    public virtual bool CanDelete(IEnumerable<TEntity> entities)
    {
        foreach (var entity in entities)
            if (CanDelete(entity)) return false;
        return true;
    }

    protected virtual bool CanDelete(TEntity entity)
    {
        return true;
    }

    public virtual void DeleteAndSave(int id)
    {
        var entity = FindOrThrow(id);
        DeleteAndSave(entity);
    }

    public virtual void DeleteAndSave(TEntity entity)
    {
        Delete(entity);
        SaveChanges();
    }

    public virtual void Delete(TEntity entity)
    {
        GetDbSet().Remove(entity);
    }

    public virtual TEntity Find(int id)
    {
        return GetDbSet().Find(id);
    }

    protected virtual TEntity FindOrThrow(int id)
    {
        return GetDbSet().FindOrThrow(id);
    }

    protected DbSet<TEntity> GetDbSet()
    {
        return DbContext.Set<TEntity>();
    }
}

ServiceBase

public abstract class ServiceBase
{
    private readonly MainDbContext _dbContext;

    protected MainDbContext DbContext
    {
        get { return _dbContext; }
    }

    public ServiceBase(MainDbContext dbContext)
    {
        _dbContext = dbContext;
    }

    protected void SaveChanges()
    {
        _dbContext.SaveChanges();
    }
}

LadderMatchService (example base class)

public class LadderMatchService : StandardService<LadderMatch, IndexModel, RowModel, DisplayModel, EditorModel>
{
    private readonly NameFinderService _nameFinderService;

    public LadderMatchService(MainDbContext dbContext, NameFinderService nameFinderService)
        : base(dbContext)
    {
        _nameFinderService = nameFinderService;
    }

    protected override IEnumerable<RowModel> ProcessRowModels(IEnumerable<RowModel> rowModels)
    {
        return rowModels.OrderBy(model => model.ReportedOn);
    }

    public override void UpdateModel(EditorModel model)
    {
        model.ChallengerNames = _nameFinderService.GetExactNameOrSimilarNames(model.ChallengerNames.Single(), false);
        model.ChallengeeNames = _nameFinderService.GetExactNameOrSimilarNames(model.ChallengeeNames.Single(), true);
    }
}
\$\endgroup\$
4
\$\begingroup\$

I think your first instinct was right. When I see

public class StandardService<TEntity, TIndexModel, TRowModel, TDisplayModel, TEditorModel> : ServiceBase
    where TEntity : class, IEntity
    where TIndexModel : IIndexModel<TRowModel>
    where TDisplayModel : IDisplayModel
    where TEditorModel : IEditorModel, new()

my nose curls at the code smell. There are far too many generics in the declaration, and a good chance it will clutter up your code later on.

If you have to have to use where with an interface, why don't you just use the interface in the class? I don't see anywhere where the classes are instantiated in your base class, and if I'm missing something, make an abstract method that returns the required class from the child class. That goes for any of the methods, use the interfaces where you can, let the sub classes take care of the rest.

Of course, I might be missing something because I haven't worked with AutoMapper before.

\$\endgroup\$
  • \$\begingroup\$ Thanks, Jeff. I new up a TEditorModel in GetEditorModel(). DbContext.Set<TEntity>() requires a class, not an interface. TRowModel is required in IIndexModel<TRowModel>. Finally, AutoMapper requires generic parameters as inputs (example: Mapper.Map<TDisplayModel>(entity)). True, I could make GetEditorModel() abstract, but I need the TEditorModel generic parameter anyway (to enable auto-mapping). I agree that the generic parameters are ugly, but I don't see a way around it without breaking up the class into multiple classes (which will result in many more classes and more code). \$\endgroup\$ – devuxer Sep 18 '12 at 5:53
  • \$\begingroup\$ What I was trying to say in my previous comment is that, if I use interfaces instead of generics, I would need to make a significant number of methods abstract, which would increase the amount of repetitive code I have to write. That said, I agree what I have now is ugly and feels wrong, and I'm still seeking ways to improve upon what I have. Please let me know if you have any other suggestions. \$\endgroup\$ – devuxer Sep 22 '12 at 17:56

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.