9
\$\begingroup\$

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?

\$\endgroup\$
  • 4
    \$\begingroup\$ This question appears to be off-topic because it is not your code that you are asking to have reviewed \$\endgroup\$ – rolfl Nov 14 '13 at 11:49
  • 1
    \$\begingroup\$ Why don't you ask the architect? They are the only person who can actually answer the question. \$\endgroup\$ – svick Nov 14 '13 at 12:26
  • \$\begingroup\$ I am asking because I want to know if I should do this myself, if this is some sort of established design pattern, or if the guy was just weird. I can't ask him, because we lost contact with him. \$\endgroup\$ – Jeff Nov 14 '13 at 12:35
  • \$\begingroup\$ - in fact the code I posted was code I wrote as I wrote this question. \$\endgroup\$ – Jeff Nov 14 '13 at 12:37
7
\$\begingroup\$

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.

| improve this answer | |
\$\endgroup\$
  • \$\begingroup\$ Considering that we have a derived implementation for each repository there is, it is unlikely that the base class will be called with 2 separate repositories - but I see where you are coming from. \$\endgroup\$ – Jeff Nov 14 '13 at 12:45
2
\$\begingroup\$

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.

| improve this answer | |
\$\endgroup\$
1
\$\begingroup\$

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).

| improve this answer | |
\$\endgroup\$
  • \$\begingroup\$ The Decorator pattern.. What exactly is it? To me it seems like a business logic layer. And yes, it was a typo. :) \$\endgroup\$ – Jeff Nov 15 '13 at 8:16
  • \$\begingroup\$ @Jeff Decorator pattern is also known as Wrapper. Intent of decorator pattern from GoF: "Attach additional responsibilities to an object dynamically. Decorators provide a flexible alternative to subclassing for extending functionality." In your case I would say you need to statically add behavior to IRepository. For details about decorator take a look en.wikipedia.org/wiki/Decorator_pattern \$\endgroup\$ – catta Nov 15 '13 at 9:55
  • \$\begingroup\$ The code was something I typed up here, so like I said it was a typo - I am wondering why he might have wanted to do it the way he did. \$\endgroup\$ – Jeff Nov 17 '13 at 15:28
  • \$\begingroup\$ @Jeff I suppose architect intention was to make InternalGet method more generic. But in my opinion it doesn't make sense because InternalGet and Get methods are coupled (by type T). This is why I see you don't need Base class. Also I "see" your architect solution as trying to solve a problem doesn't exist: prepare code for a more generic case that might come in future. If you are 100% sure you will need it soon than architect solution is good, otherwise use the simplest solution that fit your actual need and refactor later when you will need it. \$\endgroup\$ – catta Nov 20 '13 at 10:02

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.