Why would I want to always have to explicitly call a "base" method if I just want to use base functionality?

Question

I recently worked with an architect who structured his base class like this:

public abstract class Base<T>
{
    public abstract T Get(int id);

    internal T InternalGet(int id, IRepository<T> repository)
    {
       // Do more magic
       return repository.Get(id);
    }
}

And then in the derived class, if you just want to use the base functionality, you'd have to..

public class Derived : Base<T>
{
    private readonly IProductRepository _repository; // Implements IRepository<Product>

    // Ctor with dependency injection, this is good stuff!
    public DerivedClass(IProductRepository repository)
    {
       _repository = repository;
    }

    public override Product Get(int id)
    {
       // ... explicitly call base functionality - NOT a call to base though!
       return InternalGet(id, _repository);
    }
}

Why not just pass the repository to the base class, and override when you need to?

public abstract class Base<T>
{
   private readonly IRepository<T> _repository;

   protected Base<T>(IRepository<T> repository)
   {
      _repository = repository;
   }

   public virtual T Get(int id)
   {
      // Do magic
      return _repository.Get(id);
   }
}

And then in the derived class, if you don't want to use the base method, just override it.

public class Derived: Base<Product>
{
   private readonly IProductRepository _repo;

   public Derived(IProductRepository repo) : base(repo)
   {
      _repo = repo;
   }

   public override Get(int id)
   {
      if(! customerIsBeingNice) 
         throw new GoAwayException();
      return _repo.Get(id);
   }
}

I think the architect was trying to avoid the call super code smell, but since we are not requiring a call to super - in fact it really should never happen, because if you override the method, you are supposed to implement the flow you want, whereas if you call super, you can't hook into the flow that easily.

Which of these is better/worse, and why? Any alternatives?

Answer 1

The problem with building a good base class is that ideally, you should know exactly what kind of functionality will the derived classes need from the base class.

If you think they'll need more than is actually necessary, you'll write more code unnecessarily and it will also make the base class harder to use. On the other hand, if you think the derived classes will need less, your base class will be hard (or impossible) to use for those advanced scenarios.

For example, in your specific case, imagine you would need to call the base Get() with different IRepositorys. With your implementation, that's hard to do (though not impossible, you use a hack like IRepository wrapper where the wrapped IRepository can be switched, but ugh). With the architect's implementation, that's a trivial thing to do: just pass different IRepository to each call of InternalGet().

But again, choosing between the two depends on expectations of what the derived classes will need. If a scenario like I just described is unlikely, then your implementation is better, because it's simpler. But if that scenario happens, your implementation is clearly insufficient and the one from the architect is clearly better.

Answer 2

Svick gives a good explanation on why this pattern exists. It is used, when you are not sure which base functionality derived class might actually need. Using your variant makes Base class dependant on single IProductRepository injection, which might not be the case for every derived class (at least in architector's opinion). Meanwhile, your issues with this particular base class can be easily solved by adding another base class to this hierarchy:

public abstract class SingleRepositoryBase<T> : Base<T>
{
    public override T Get(int id)
    {
        return InternalGet(id, _repository);
    }

    protected SingleRepositoryBase(IRepository<T> repository)
    {
        _repository = repository;
    }

    private readonly IRepository<T> _repository;
}

This will nicely cover both cases, and in my experience - that is how this patterns normally evolves.

Answer 3

Based on your code example I will say something is missing from picture, but I give it a try. I have a different view from you and architect.

First some comments on architect code version: In Base class I will make InternalGet method protected, not internal. This way it is visible to only derived classes no matter in what assembly (suppose your solution has many projects and you need to have derived classes in different projects). Derived class should be defined like Derived : Base< Product >, other wise you have a compile error (type T definition is missing). Looks like a typo mistake, this ring a bell to me something is missing from picture.

Because in base class the abstract method Get(int id) is identical with Get(int id) in IRepository and because Get and InternalGet methods both are coupled to type T I will suggest to use Decorator pattern to decorate IRepository (I don't see how InternalGet can deal with a repository for a different type that method Get return). Also because the "do more magic" is in Base class looks like the magic is kind of generic implementation. I would give a try to a implementation with generics. Here is my suggestion:

Decorator:

    public class RepositoryDecorator<T> : IRepository<T>
    {
        private readonly IRepository<T> _decoreatedRepository;

        public RepositoryDecorator(IRepository<T> decoreatedRepository)
        {
            _decoreatedRepository = decoreatedRepository;
        }

        public T Get(int id)
        {
            // do more magic
            return _decoreatedRepository.Get(id);
        }
    }

Client class:

    public class RepositoryClient
    {
        private readonly IRepository<Product> _productRepository;

        public RepositoryClient(IRepository<Product> decoreatedRepository)
        {
            _productRepository = new RepositoryDecorator<Product>(decoreatedRepository);
        }

        public void Do(int id)
        {
            // productRepository is a decorator, but it's implementation type is hidden 
            Product product = _productRepository.Get(id);

            // use product
        }
    }

RepositoryDecorator implements IRepository interface, this is need it to expose the decorator with same type as the decorated repository.

RepositoryDecorator can be generic. If "do more magic" is specific to type T than you can have a decorator for each particular T type (like one for Product). RepositoryClient use a RepositoryDecorator actually, in my example decorator is instantiated in client constructor, but this is implementation detail. Decorator can be created outside and injected in client constructor as a simply IRepository. Client would not know he is dealing with a decorator in reality.

Again, my suggestion is based on your succinct code. Your requirement could be more complex, in that case it will lead you to many decorators (per decorated type T).