2
\$\begingroup\$

I have a Visual Studio Solution which has a bunch of Projects in it. One of these projects is called "Services" and is basically the junction point between all remaining projects. When I built it originally I made it as generic as possible so I can expand on it. At the time I only had to work with Entity Framework backed implementations of it, but now I've added QuickBooks to the mix as well. It mostly worked out well, but I saw the need to refactor and redesign bits and pieces of it. To that end I've spent most of yesterday doing that and I have new code that takes care of the weirdness I had before, but, I find myself using lots of interfaces and lots of inheritance and I'm not exactly comfortable with it. For example here's my "DbService" implementations:

public class DbService<TEntity> :
    Service<TRepository>,
    IDbQueryableService<TEntity>,
    IQueryableService<TEntity> {}

public class DbService<TEntity, TKey> :
    DbService<TEntity>,
    IDbGettableService<TEntity>,
    IGettableService<TEntity, TKey>,
    IRemovableService<TEntity, TKey>,
    IRestorableService<TEntity, TKey> {}

public class DbService<TEntity, TEntityFormItem, TKey> :
    DbService<TEntity, TKey>,
    IAddableService<TEntityFormItem, TKey>,
    IUpdatableService<TEntityFormItem, TKey> {}

public class DbService<TEntity, TEntityFormItem, TEntityListItem, TKey> :
    DbService<TEntity, TEntityFormItem, TKey>,
    IDbListableService<TEntityListItem>,
    IListableService<TEntityListItem> {}

public class DbService<TEntity, TEntityFormItem, TEntityListItem, TEntitySelectItem, TKey> :
    DbService<TEntity, TEntityFormItem, TEntityListItem, TKey>,
    IDbSelectableService<TEntitySelectItem>,
    ISelectableService<TEntitySelectItem> {}

This is very similar to my current code and allows me to only inherit from a base class that has the functionality I want. For example, if a TEntity can be listed, but doesn't need to be displayed as a select list, then I'll inherit from the fourth DbService. This way I know I can upgrade or downgrade my inheritance as I need to. That being said, it itself has a lot of inheritance between base classes and interfaces. There's six levels of base class inheritance and 12 interfaces.

So, this morning I tried to bring it all together a little bit and came up with this:

public class DbService2<TEntity, TEntityListItem, TEntitySelectItem, TKey> :
    Service<TRepository>,
    IDbReadableService<TEntity, TEntityListItem, TEntitySelectItem>,
    IReadableService<TEntity, TEntityListItem, TEntitySelectItem, TKey> {}

public class DbService2<TEntity, TEntityFormItem, TEntityListItem, TEntitySelectItem, TKey> :
    DbService2<TEntity, TEntityListItem, TEntitySelectItem, TKey>,
    IWritableService<TEntity, TEntityFormItem, TKey> {}

Now this version obviously reduces the depth of inheritance on the base classes to three and the interfaces to three, but, it forces me in a position where some inheriting TEntity services will not be using all of the features so I'd have to use placeholders to fill in some of the TEntity* parameters and that feels like it will end up dirtier in the long run than the first code sample.

Also, I do realize that I'm using lots of generic parameters, but it works for my needs and this not being a publicly exposed API, I'm OK with it.

So, my question for you fine folks is, am I being too zealous in trying to reduce the base class and interface inheritances or am I OK to continue as is, which has worked so far (referring to the original code I was refactoring which is very close to the first example)? Visual Studio gives the first example a maintainability index of 86.5% (average), and 82.5% (average) for the second example, so if I follow VS assessment, I should continue with the first example?

\$\endgroup\$

closed as off-topic by IEatBagels, Mathieu Guindon, Simon Forsberg, 200_success Aug 25 '14 at 18:49

This question appears to be off-topic. The users who voted to close gave this specific reason:

  • "Questions containing broken code or asking for advice about code not yet written are off-topic, as the code is not ready for review. Such questions may be suitable for Stack Overflow or Programmers. After the question has been edited to contain working code, we will consider reopening it." – IEatBagels, Mathieu Guindon, 200_success
If this question can be reworded to fit the rules in the help center, please edit the question.

  • 3
    \$\begingroup\$ Rather than inheritance could you use composition instead, so nothing inherits from DbService, but instead they all contain private variables of type DbService?? \$\endgroup\$ – dreza Jul 5 '14 at 21:54
  • \$\begingroup\$ Ok, so I spent most of the day reading up on composition, and it looks promising. From what I've understood, I should end up creating a *Service<*> for each interface and then inject these as needed into to actual TEntityService classes. I figure there's also a possibility of using inheritance still on the individual interface specific classes (as appropriate). I would have to also create TEntity specific implementations for some interfaces because they won't follow the base logic. How would I in-turn inject the specific implementations where appropriate? I'm using Ninject in an MVC app. \$\endgroup\$ – Gup3rSuR4c Jul 6 '14 at 2:50
  • \$\begingroup\$ I guess I could just specify the actual specific implementation in the constructor of the TEntityService class, but then that kind of defeats the purpose of using the interfaces for injection. \$\endgroup\$ – Gup3rSuR4c Jul 6 '14 at 2:53
  • \$\begingroup\$ Does your classes not have any methods in them? Are you getting all methods from inheritance? \$\endgroup\$ – Simon Forsberg Aug 25 '14 at 18:34
  • \$\begingroup\$ I think it's a bit hard to review code like this because the code you are showing looks a bit like example code, how does the classes you are inheriting from look? Unlike Stack Overflow, we prefer to look at wall of text code rather than stripped down examples. \$\endgroup\$ – Simon Forsberg Aug 25 '14 at 18:37
1
\$\begingroup\$

Zealous nitpicking...

am I being too zealous in trying to reduce the base class and interface inheritances

Careful with wording here: a type inherits from a base class, and implements an interface. Interfaces aren't inherited, they're implemented. Subtle, but important distinction.


My two cents...

Looking at the base class in the first snippet:

public class DbService<TEntity> :
    Service<TRepository>,
    IDbQueryableService<TEntity>,
    IQueryableService<TEntity> {}

Can you spot the code smell?

What's the difference between these two interfaces?

    IDbQueryableService<TEntity>
    IQueryableService<TEntity>

You're not listing any of these interfaces, but just from the names one can tell that IQueryableService<TEntity> and IDbQueryableService<TEntity> probably overlap in functionality: they're both queryable services.

Leaky abstractions

What you have here is a leaky abstraction: you're making an abstraction (/interface), but the implementation is leaking into the abstraction - whether the data is coming from a database, from a web service, or from text files, shouldn't matter; you want an abstraction for something that can be queried... and IQueryableService<TEntity> seems to accomplish that. IDbQueryableService<TEntity> is leaking a database implementation of an IQueryableService<TEntity>.

IDbQueryableService<TEntity> is redundant and should be removed.

Same goes for IDbGettableService<TEntity>, which is leaking a database implementation of IGettableService<TEntity, TKey>, at least so it seems from the interfaces' names.

Consistency in design

Where's IDbRemovableService<TEntity>? Where's IDbAddableService<TEntity>? Where's IDbUpdatableService<TEntity>? Why aren't these operations paired with a non-IDbSomething interface like the queryable and gettable stuff?


I would have lots of other questions about why you need to have a TEntityListItem (among other things), but there's really not enough context in your question to pursue any further without assuming what your implementations do, and what methods your interfaces expose.

I suggest you post another question with your actual code, so reviewers can actually go over your code and make sound recommendations. Just reviewing class names and interfaces without even having the interface members, is pretty hard.

I would venture to say that I highly doubt you need that many generic type parameters, but without knowing more about your code all I can do is say "I don't like this"; whether it's a private API or publicly exposed to client code doesn't matter: it's code that you need to maintain and understand, now and in the future - I don't care what Visual Studio thinks the "maintainability index" is (especially when anything between 20 and 100 is "green"). Having 12 different interfaces that, together, seem to be doing what a single repository could do, doesn't feel right. In fact, the first impression I got was "boy this is over-engineered". And I'm sure the impression wouldn't go away by looking at your actual code.

\$\endgroup\$

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