Ayende @ Rahien

It's a girl

Code Review Guidelines: Avoid inheritance for properties

I recently have gone over some codebase to find something like this:

public interface IAuditable
{
  DateTime UpdatedAt {get;set;}
  string UpdatedBy {get;set;}
  DateTime CreatedAt {get;set;}
  string CreatedBy {get;set;}
}

public interface IEntity
{
  int Id {get;set;}
}

public class Entity : IEntity
{
  public int Id { get;set; }
}

public class AuditableEntity : Entity, IAuditable
{
  public DateTime UpdatedAt {get;set;}
  public string UpdatedBy {get;set;}
  public DateTime CreatedAt {get;set;}
  public string CreatedBy {get;set;}
 
}

public class Visit : AuditableEntity
{
  // stuff
}

I look at code like that, and it is more than a bit painful. It is painful, because this sort of code is badly abusing inheritance.

The problem is that this is mostly intended to save on typing, but with things like automatic properties, there isn’t really much point here. What it does produce is code that seems to be more complicated than it is, because now we have those classes in the middle that does nothing but provide properties for you to use. Worse than that, they take down the only base class slot that you have, and they force you to think in a way that isn’t always natural.

It is just as easy, and much clearer to use:

public interface IAuditable
{
  DateTime UpdatedAt {get;set;}
  string UpdatedBy {get;set;}
  DateTime CreatedAt {get;set;}
  string CreatedBy {get;set;}
}

public interface IEntity
{

}

public class Visit : IAuditable, IEntity
{
  public int Id {get;set;}
  public DateTime UpdatedAt {get;set;}
  public string UpdatedBy {get;set;}
  public DateTime CreatedAt {get;set;}
  public string CreatedBy {get;set;}
 
}

And hey, you can now have an auditable that have a composite key, something that you used to need a completely separate inheritance hierarchy to deal with.

Comments

zdeslav
02/21/2011 04:33 PM by
zdeslav

The problems start when all your 54 implementations of IEntity have to be modified because you have added IsPersistent() to IEntity...

I have implemented lot of stuff using ATL and could never understand the fears of multiple inheritance (given that you have competent developers). C# (.NET, actually) needs at least proper mixins

oleksii
02/21/2011 04:42 PM by
oleksii

Ayende,

I am not sure but I thought that having an empty "pointer" interface is not the best solution. What if, there are many domain entities that actually use Id property. Following your improvement, all those entities would end up duplicating Id property. What I suggest is removing IEntity and making Entity abstract to preserve Id in the base class. I am not sure however about the composite key. Are there any other ways to have a composite key with a base entity class?

Thanks,

Oleksii

Mark Lee
02/21/2011 06:01 PM by
Mark Lee

Perhaps you could have an abstract base Entity class, with an Id property, instead of an empty IEntity. The Id property would be of type Identity, which is the root of another inheritance hierarchy. Or Entity could be a generic with the Id type being the generic parameter (possibly specifying that it must implement an interface or derive from a type, so that Entity knows how to interact with it). It really depends on the requirements of the system though.

Also, I'm not sure about this way of thinking where a class's state is exposed directly through properties by default. The class should modify its own state internally as needed and should only provide read access to it through a well-defined operation. The way this code is written, any code that uses an entity can modify its creation time or id arbitrarily.

zdeslav
02/21/2011 08:09 PM by
zdeslav

I don't see how having abstract base class would solve the main problems of wasting the only inheritance slot and being unable to combine boilerplate implementations to build a new type.

On few occasions where it was really necessary, I solved these issues by injecting mixin class via DynamicProxy, but it's far from simple solution, at least from maintenance POV

Magnus
02/21/2011 08:10 PM by
Magnus

I agree, that if it is solely to reduce on typing, it is a bad idea.

IMO, it quickly shifts if there is logic as well, though. Like wiring of validation-logic, overridden equality members etc. But I guess that is not what you are addressing here :-).

Belvasis
02/21/2011 08:26 PM by
Belvasis

I don't understand what you want to say with it. Sure you are right if the model simply is just like this. But usually a base Entity class has a lot more to offer than a simple ID property...Equals, GetHashCode etc.

So how would you do that without an Entity base class?

Ayende Rahien
02/22/2011 05:49 AM by
Ayende Rahien

Zdeslav,

Modifying stuff like IEntity happens very rarely, and even when it does...

Well, spending 5 - 10 minutes doing this is a piece of cake, well worth the codebase that it produces.

As for mixins, I would agree, but since we don't have it...

Ayende Rahien
02/22/2011 05:51 AM by
Ayende Rahien

Oleksii,

This isn't an empty interface, there is usually some code making use of that.

As for "duplicating the Id property", how is this bad in any way? It is a single liner, and it reduce dependencies across the entire codebase.

Ayende Rahien
02/22/2011 05:52 AM by
Ayende Rahien

Mark Lee,

Do you realize how complex your design is getting now? We are now talking about multiple levels of hierarchies, usually with generics thrown in as well, all to support something that isn't really valuable in the first place.

Dmitry
02/22/2011 06:36 AM by
Dmitry

The issue is you often needs to override Equals, GetHashCode, ToString and possibly have add common methods based on some of the common properties.

Andrey Shchekin
02/22/2011 07:09 AM by
Andrey Shchekin

I completely agree with this post (and btw I do not see why have Id at all, NH does not require that).

For other comments -- are Equals/GetHashCode actually the same for all entities? Do you just use Id in them? If so, why -- with Identity Map default Equals and default GetHashCode should do exactly the same thing?

oleksii
02/22/2011 08:57 AM by
oleksii

Ayende,

Thanks for the reply.

What do you think about Open-Closed principle (for updating the interface) and DRY (for ID)? Would you agree with Belvasis that Entity usually would have some other useful methods? Are there real reduction in terms of dependancies whether it is an abstract class or an interface? Still one depends on abstraction. Futhermore, if one casts the object to the interface type IEntity, there is nothing in it, so it is just an indicator that a class belongs to some hierarcy, which is IMO better shown by an abstract class.

Ayende Rahien
02/22/2011 09:01 AM by
Ayende Rahien

Oleksii,

GetHashCode/Equals - with identity map - and you should be using one, are not needed.

I don't see anything else that the Entity should have.

Properties definition don't follow DRY.

Bikal Gurung
02/22/2011 09:50 AM by
Bikal Gurung

Ayende,

I would say that your refactored code still has some code smells. It would be much cleaner, concise and DRY if we can remove the usage of interfaces all together. We are repeating the same set of properties in the interface and the class that inherits from it. In my opinion, abstracting the properties in the interface doesn't serve as a useful unique abstraction but more of a noise and adds to the complexity and maintenance overhead. Not to mention refactoring is a pain, ie. If I were to change a property name in the interface that change would have to be applied to all the classes inheriting from the interface. This is a clear sign of a code smell and indicative of a not well designed system. So looking at the code above, I would refactor the code base as follows,

public class Entity

{

public int Id {get;set;}

public DateTime UpdatedAt {get;set;}

public string UpdatedBy {get;set;}

public DateTime CreatedAt {get;set;}

public string CreatedBy {get;set;}

}

public class Visit : Entity

{

// visit specific properties

}

The above code is concise and the abstraction is much meaningful and DRY.

Bikal

Ayende Rahien
02/22/2011 10:53 AM by
Ayende Rahien

Bikal,

Now you can't have a Auditable Entity with Composite Key

Koen Metsu
02/22/2011 10:59 AM by
Koen Metsu

In my opinion: the DRY principle is not about not coding method/property signatures twice, but about not coding your logic in more than one place.

Bikal Gurung
02/22/2011 11:46 AM by
Bikal Gurung

Hi Ayende,

I think one can have composite key in the scenario that I mentioned by adding another primary key property - perhaps called VisitID - and do the composite key mapping in separate classes(ala fluent nhibernate) or hibernate mapping files. This could also most likely lead to a better OO abstraction and domain modeling since the domain model abstraction and its mapping concerns are kept separate.

@Koen Metsu,

I agree. DRY is much more than removing the repetition of methods/properties signature twice. But when we do repeat the methods/properties twice it is a sure sign of code smell and an indication that you need to refactor your code to remove the repetition and redundancies.

This is especially so because say we need to change the name of the property in our base interface and this interface is implemented/inherited by 50 domain classes. Then we need to change the property/method signature in 50 classes. What if we add another new property in the base interface ?? Surely, this is indicative that your code isn't DRY.

The pain point I described above can be easily avoided if we are to use class inheritance rather than interface inheritance. Because of this reason interface inheritance use - in my opinion - should be avoided as much as you possible can in favour of class based inheritance.

Koen Metsu
02/22/2011 12:09 PM by
Koen Metsu

Hi Bikal,

How would you go about extending a property getter, for example to implement logging/Lazy Loading/INPC, specifically for that subclass then?

When you add/rename/remove an abstract method to your base class, you'd also have to alter it in all of your subclasses. Still we don't see abstract methods as a violation of dry?

Bikal Gurung
02/22/2011 12:29 PM by
Bikal Gurung

Hi Koen,

I think we are going slightly off topic here. Inheritance by design is meant to remove repetitive/redundant code. Class inheritance solves this pain point nicely and makes our code more DRY. This is not the case with interface inheritance as illustrated in my previous comments. This is my main assertion here - using class based inheritance - as opposed to interface based inheritance - to make our code more DRY.

With class based inheritance we don't have to modify and or alter our subclasses - the side effect of refactoring is limited only to the base class and not the subclasses. This is not the case in interface based inheritance and is clearly a violation of DRY principles - that is one change propagates to many.

With regards to abstract method use, I would think this is more in the realm of dynamic binding and polymorphism rather than DRY principles since abstract method by their very use indicates subclasses will have unique and different implementations of a given method and as such no repetition takes place.

Gregory
02/22/2011 12:40 PM by
Gregory

I find DRY vastly overrated and overused. A question to ask is "what are you doing?" If you are doing the Exact same thing (e.g. the same logic in the same context for the same purpose) then repeating yourself is a smell. All too often people mistake similar logic as repetition.

As for Inheritance vs composition I find a good question to ask is this, "Do I care what an object Is or what it Does?"

@Ayende - I like your solution because (generally speaking) most of the applications I create would want objects to have Auditing and Entity behaviors (regardless of what they 'really' are). Whereas a Visit might have a specific meaning and purpose in the application. If even Visit is without application meaning then I would imagine there is another interface (or two) with those behaviors that should be implemented.

@Bikal - If you are proposing an application in a context where a Visit object Is an Entity (and there is a unique and/or valuable meaning to that) then I think your solution is acceptable. To put it another way - if significant parts of your application should work with an Entity object because that is the primary domain abstraction you may have a point. If the only parts of your application that would be concerned with the Entity object are cross-cutting infrastructure objects (logging, persistence, etc.) I think it more likely you are using inheritance as a language trick to reduce keystrokes - not so acceptable.

Bikal Gurung
02/22/2011 01:05 PM by
Bikal Gurung

@Gregory,

Inheritance to reduce keystrokes? It seems you have over simplified the use of inheritance. Use class based inheritance to reuse the same abstraction again and again without side effects. The abstraction can include property/methods/fields and other logic you deem fit. A perfect analogy to understand inheritance would be a developer doing improved version of copy/paste of a piece of code again and again but without the side effects of doing the copy/paste.

The side effects of normal copy/paste can be manifested in several ways namely in the copy/paste action itself and also if the original method that is being pasted across is buggy. The latter side effect in particular is very unproductive since the same bug is magnified to many places and fixing this bug has to be done in that many areas in your code. Inheritance avoid this pain point because the change is limited to only the base class. Is this just saving keystrokes and or an overuse of DRY principles? I think not.

driesie
02/22/2011 01:24 PM by
driesie

Why not use composition?

Along the lines of

public interface IAuditInfo

{

    DateTime UpdatedAt { get; set; }

    string UpdatedBy { get; set; }

    DateTime CreatedAt { get; set; }

    string CreatedBy { get; set; }

}


public interface IAuditable

{

    AuditInfo Audit { get; set; }

}


public interface IEntity

{


}


public class Visit : IAuditable, IEntity

{

    public int Id { get; set; }

    public IAuditInfo Audit { get; set; }

}
joshka
02/22/2011 03:04 PM by
joshka

+1 for composition.

Gregory almost hit the nail on the head.

class Visit /* or Entity */ {

AuditingBehavior _auditingBehavior;

void Foo() {

 // ...

_auditingBehavior.MarkUpdated();

}

}

tawani
02/22/2011 05:02 PM by
tawani

This article sounds like you are "Database Mapping First" thinking. Instead of "Code/Logic First" thinking.

Composition is even better, but for simplicity, just use an Abstract Entity

meisinger
02/22/2011 08:12 PM by
meisinger

@Bikhal - um... inheritance is not meant to remove repetitive/redundant code; inheritance is meant for classification

@Gregory - thanks for indicating the overuse of DRY. i have seen too many "extreme" DRY implementations that have drove me nuts

@Ayende - so basically; if you chew up the inheritance slot you are potentially coding yourself into a corner but with an interface you are free to implement what your model needs

is an abstract implementation too YAGNI?

Scooletz
02/22/2011 10:56 PM by
Scooletz

One of the most interesting programmers, Greg Young, on the very last presentation published on the InfoQ said that he used the inheritance (not implementation of an interface) twice across last a few months. And this is good! I've seen projects where derivation became the way to only spend a few seconds less in writing properties implementations. Having Resharper with its good option of auto implementation lowers the cost of writing to zero, hence, reducing the need of loosing the only one baseclassslot. All derivation tree should be cut if not needed, otherwise you can get a forrest with not so different trees (beside their roots with id types, for instance).

zdeslav
02/23/2011 11:12 AM by
zdeslav

It seems that the discussion has moved to general talk about use of inheritance, but the original context is about inheritance limitations in .NET.

In C++ you could use multiple inheritance for this, and it would be perfectly fine: Visit is an entity and it is auditable. Therefore, it would be completely fine to provide 2 boilerplate implementations of IEntity and IAuditable and derive from that.

In .NET, if you really need to split different functionality into separate interfaces, which you should (SOC, SRP), and you need to implement these interfaces in bunch of classes, doing it as described here is technically correct, but also pain in the ass, and developers will turn to approach which seems more productive.

Other languages/technologies solve this in different tools. Unfortunately, .NET lacks a proper solution for this (well, I don't consider IL rewriting, mixin injection and similar tools as proper solution)

Ayende Rahien
02/23/2011 11:15 AM by
Ayende Rahien

Meisinger,

That is one of the issues, yes. Another was a big inheritance hierarchy that served only to save some typing.

Wayne Molina
02/27/2011 04:35 AM by
Wayne Molina

I'm on the fence with this. On one hand I see the point of not doing the typical "Interfaces for everything, just because" approach which IMO stinks of cargo-cult programming. But on the other hand it doesn't "feel" right to be duplicating logic in entity classes that are common across ALL entities; that seems to be the entire purpose of inheritance in the first place.

I suppose the argument is that who cares about DRY for simple properties that have no logic whatsoever - DRY does apply to logic and not the code itself; I would still be in favor of the first example if and only if the classes had actual business logic that was common across all of them, because that IS violating DRY since you would have to repeat the same logic in all your entities.

I also have to say I am not wholly against "marker interfaces" that do nothing and are just used to test if something can perform a type of action, but they have to be used sparingly.

Al Todd
02/28/2011 11:45 AM by
Al Todd

A big point has been missed here.

Adding audit fields to every database table is inherently WET (why every table?). Its a hangover of optimisation runs.

Auditing is x-cutting. We therefore needs some AOP to solve it, and driven by requirements. I should simply need to be able to add an attribute such as

[Auditable(CreatedDate | UpdatedDate)]

to my class and let the underlying aspect handle this responsibility.

Raciel
03/03/2011 12:22 PM by
Raciel

People tend to abuse of inheritance because they want to create models as close to the reality as possible.

You stop abusing of inheritance when you hit complex domains and realize that inheritance introduces strong coupling in your model.

On those cases you usually find composition (through interfaces + extension methods) to be more flexible and helpful. (I wish we have the option for extension properties too).

The key IMO should be creating useful abstractions... rather than trying modeling reality.

Francois Germain
03/07/2011 01:48 PM by
Francois Germain

Talk about simplicity...

Why just never use composite key in your object design, especially for a system that must support full data auditing on CRUD and more? Having a surrogate key on every object makes life MUCH easier across the board...

We just spent a couple months writing an audit system on top of EF that scales well to up to 500K audits per entity. I really don't know how we'd get to that design without surrogate IDs.

-f

Comments have been closed on this topic.