3
\$\begingroup\$

I have been using Entity Framework 6, AutoMapper, ASP.NET Web Api and hit a few problems along the way, each time it was recommended that I could change my code from Inheritance to use composition design instead. After the last problem, and since the software is in early stages, I thought I would try the composition method and see how it panned out. Now I know that composition can mean that you write out methods multiple times, unlike inheritance, but I understand the tradeoffs and I think it's fine, but I wonder if I have missed something here.

I have an EntityBaseService which was trying to deal with an object sometimes needing validation. Using inheritance I would have:

public interface IEntityBaseService<TEntity> 
    where TEntity : class, IEntityBase
{
    Task<ICollection<TEntity>> GetAllAsync();

    Task<TEntity> GetAsync(long id);

    Task<ValidationResult> ValidateAsync(TEntity t);

    Task<int> AddAsync(TEntity t);

    Task<int> UpdateAsync(TEntity updated);

    Task<int> DeleteAsync(TEntity t);

}

public abstract class EntityBaseService<T> : IEntityBaseService<T>
    where T : class, IEntityBase
{
    protected IDataContext _context;
    protected IValidator<T> _validator = null;

    protected EntityBaseService(IDataContext context)
    {
        _context = context;
    }

    protected EntityBaseService(IDataContext context, IValidator<T> validator)
        : this(context)
    {
        _validator = validator;
    }

    public virtual async Task<ICollection<T>> GetAllAsync()
    {
        return await _context.Set<T>().ToListAsync();
    }

    public virtual Task<T> GetAsync(long id)
    {
        return _context.Set<T>().Where(e => e.Id == id).FirstOrDefaultAsync();
    }

    public virtual Task<ValidationResult> ValidateAsync(T t)
    {
        if (_validator == null) throw new MissingFieldException("Validator does not exist for class " + t.GetType().ToString() + ". override method if no validation needed");
        return _validator.ValidateAsync(t);
    }

    public virtual async Task<int> AddAsync(T t)
    {
        var results = await ValidateAsync(t);

        if (!results.IsValid) {
            throw new ValidationException(results.Errors);
        }

        if (_context.GetState(t) == EntityState.Detached)
        {
            _context.Set<T>().Add(t);
            _context.SetState(t, EntityState.Added);
        }

        return await _context.SaveChangesAsync();
    }

    public virtual async Task<int> UpdateAsync(T updated)
    {
        var results = await ValidateAsync(updated);

        if (!results.IsValid)
        {
            throw new ValidationException(results.Errors);
        }

        if (_context.GetState(updated) == EntityState.Detached)
        {
            _context.SetState(updated, EntityState.Modified);
        }

        return await _context.SaveChangesAsync();
    }

    public virtual Task<int> DeleteAsync(T t)
    {
        _context.SetState(t, EntityState.Deleted);

        return _context.SaveChangesAsync();
    }
}

And then another class, would inherit from this service, like so, with its own interface:

public interface IAttachmentService : IEntityBaseService<Attachment>
{
    Task<AttachmentData> GetDataAsync(long id);
}

public class AttachmentService : EntityBaseService<Attachment>, IAttachmentService
{
    public AttachmentService(IDataContext context, AttachmentValidator validator) : base(context, validator) { }

    public async Task<AttachmentData> GetDataAsync(long id)
    {
        return await _context.Attachments
            .Where(e => e.Id == id)
            .Select(e => e.Data)
            .FirstOrDefaultAsync();
    }

}

As I said, a lot of methods I were overwriting (unlike this example). Trying composition, I have easily separated the logic of requiring validation and not, since I have now created a ValidationService, which can be added to an object that needs it:

public interface IEntityBaseService<T> where T : class, IEntityBase
{
    IDataContext Context { get; }

    Task<ICollection<T>> GetAllAsync();

    Task<T> GetAsync(long id);

    Task<int> AddAsync(T entity);

    Task<int> UpdateAsync(T entity);

    Task<int> DeleteAsync(T entity);
}

public class EntityBaseService<T> where T : class, IEntityBase
{
    protected IDataContext _context;

    public IDataContext Context
    {
        get
        {
            return _context;
        }
    }

    protected EntityBaseService(IDataContext context)
    {
        _context = context;
    }

    public virtual async Task<ICollection<T>> GetAllAsync()
    {
        return await _context.Set<T>().ToListAsync();
    }

    public virtual Task<T> GetAsync(long id)
    {
        return _context.Set<T>().Where(e => e.Id == id).FirstOrDefaultAsync();
    }

    public virtual async Task<int> AddAsync(T entity)
    {
        if (_context.GetState(entity) == EntityState.Detached)
        {
            _context.Set<T>().Add(entity);
            _context.SetState(entity, EntityState.Added);
        }

        return await _context.SaveChangesAsync();
    }

    public virtual async Task<int> UpdateAsync(T entity)
    {
        if (_context.GetState(entity) == EntityState.Detached)
        {
            _context.SetState(entity, EntityState.Modified);
        }

        return await _context.SaveChangesAsync();
    }

    public virtual Task<int> DeleteAsync(T entity)
    {
        _context.SetState(entity, EntityState.Deleted);

        return _context.SaveChangesAsync();
    }
}

Validation Service, now separated:

public interface IValidationService<T> where T : class, IEntityBase
{
    Task<ValidationResult> ValidateAsync(T entity);

    Task<int> AddAsync(T entity);

    Task<int> UpdateAsync(T entity);
}

public class ValidationService<T> where T : class, IEntityBase
{
    private EntityBaseService<T> _service;
    private IValidator<T> _validator = null;

    public ValidationService(EntityBaseService<T> service, IValidator<T> validator)
    {
        _service = service;
        _validator = validator;
    }

    public Task<ValidationResult> ValidateAsync(T t)
    {
        if (_validator == null) throw new MissingFieldException("Validator does not exist for class " + t.GetType().ToString() + ". override method if no validation needed");
        return _validator.ValidateAsync(t);
    }

    public virtual async Task<int> AddAsync(T t)
    {
        var results = await ValidateAsync(t);

        if (!results.IsValid)
        {
            throw new ValidationException(results.Errors);
        }

        return await _service.AddAsync(t);
    }

    public virtual async Task<int> UpdateAsync(T updated)
    {
        var results = await ValidateAsync(updated);

        if (!results.IsValid)
        {
            throw new ValidationException(results.Errors);
        }

        return await _service.UpdateAsync(updated);
    }
}

Now a service that "CAN" implement the methods I need, with the logic I require for that object can look like this:

public interface IAttachmentService
{
    Task<Attachment> GetAsync(long id);

    Task<int> CustomAsync(long id);

    Task<int> AddAsync(Attachment entity);

    Task<int> UpdateAsync(Attachment entity);

    Task<int> DeleteAsync(Attachment entity);

}

public class AttachmentService : IAttachmentService
{
    private IEntityBaseService<Attachment> _service;
    private IValidationService<Attachment> _validation;

    public AttachmentService(IEntityBaseService<Attachment> service, IValidationService<Attachment> validation) {
        _service = service;
        _validation = validation;
    }

    public Task<Attachment> GetAsync(long id)
    {
        return _service.GetAsync(id);
    }

    public async Task<int> CustomAsync(long id)
    {
        // custom code
        return _service.Attachments.Where(e => e.Number > id).FirstOrDefaultAsync();
    }

    public Task<int> AddAsync(Attachment entity)
    {
        return _validation.AddAsync(entity);
    }

    public Task<int> UpdateAsync(Attachment entity)
    {
        return _validation.UpdateAsync(entity);
    }

    public Task<int> DeleteAsync(Attachment entity)
    {
        return _service.DeleteAsync(entity);
    }

}

Now is this right? I believe it is, but the inheritance part of my brain is saying that I need to somehow not have to create GetAsync, AddAsync, UpdateAsync etc in each object, but without doing so means that I cannot customize a function easily, without writing an override. Plus this all seems easily testable now, as I can test each service independently using a testing framework.

\$\endgroup\$
3
  • \$\begingroup\$ I think that what's confusing you and smelling bad to me is that your AttachmentService is wrapping all the method calls from two classes that have different goals. Your class is composed of parts, and those parts do what they need to do. You can make those into public properties (or { get; private set; } if you prefer) and remove all the wrapping methods. \$\endgroup\$
    – Magus
    Apr 16, 2015 at 19:04
  • \$\begingroup\$ Not sure i understand u? Isnt composition all about including classes in other classes? Can u give me example of how u would do it so i can understand more? \$\endgroup\$
    – Gillardo
    Apr 16, 2015 at 20:06
  • \$\begingroup\$ It is about including instances of classes in other classes, but it isn't about duplicating the methods of children. I've answered with a simple demonstration. \$\endgroup\$
    – Magus
    Apr 16, 2015 at 20:43

1 Answer 1

1
\$\begingroup\$

One simple thing you could do to make things simpler and less repetitive, while staying more in line with the idea of composition is this:

public class AttachmentService
{
  public IEntityBaseService<Attachment> EntityService { get; private set; }
  public IValidationService<Attachment> ValidationService {get; private set; }

  public AttachmentService(IEntityBaseService<Attachment> entityService, IValidationService<Attachment> validationService)
  {
    EntityService = entityService;
    ValidationService = validationService;
  }
}

What this means is that AttachmentService is now truly a composite. You don't need to create methods to wrap the parts' methods, because the parts' methods can be called directly. (Instead of attachmentService.GetAsync(3), you would now call attachmentService.EntityService.GetAsync(3), because the attachment service has no need to know what methods it's parts contain, just that it has parts)

Composition is not really about duplicating methods; done correctly, it results in far less code, and far simpler classes, because each class does only what it needs to do, and can be substituted for other implementations if needed.

\$\endgroup\$
5
  • \$\begingroup\$ Its a fine line, and takes some discipline to keep the chaining from devolving into chaos, but I think you're right in this case. \$\endgroup\$
    – RubberDuck
    Apr 16, 2015 at 22:13
  • 1
    \$\begingroup\$ @RubberDuck: Agreed. To clarify for the OP, if there is repetitive chaining at the next level up, I'd create methods on that level to do those simple chains. The only methods that really belong in this class are interactions between the two services or operations an AttachmentService itself can do - whatever those may be. \$\endgroup\$
    – Magus
    Apr 16, 2015 at 22:24
  • \$\begingroup\$ Thanks for the example but a question. If i do not add an actual method for addasync which calls the validationservice.addasync method then i cannot force validation when an attachment is added because someone could use the entitybaseservice.addasync instead? \$\endgroup\$
    – Gillardo
    Apr 17, 2015 at 5:29
  • \$\begingroup\$ @user2736022: Then what you have is a major design issue. Your services, which presumably do different things, appear to do the same thing. If they are not meant to both be used that way, they should not both provide that ability. You should read up on SOLID; the Wikipedia article does a very good job of explaining it, and it should help you considerably. \$\endgroup\$
    – Magus
    Apr 17, 2015 at 14:54
  • \$\begingroup\$ Think your right, i have taken this too far. I think each service should of actually inherited from EntityBaseService, and ValidationService should be passed into each service that requires it. Thanks \$\endgroup\$
    – Gillardo
    Apr 17, 2015 at 15:32

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service and acknowledge that you have read and understand our privacy policy and code of conduct.

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