Is it normal to have a base type for Entities and Value Objects?

Question

Please see the code below, which I am currently using in a production application. :

public abstract class Entity
{
    public virtual long Id { get; protected set; }
 
    public override bool Equals(object obj)
    {
        var compareTo = obj as Entity;
 
        if (ReferenceEquals(compareTo, null))
            return false;
 
        if (ReferenceEquals(this, compareTo))
            return true;
 
        if (GetRealType() != compareTo.GetRealType())
            return false;
 
        if (!IsTransient() && !compareTo.IsTransient() && Id == compareTo.Id)
            return true;
 
        return false;
    }
 
    public static bool operator ==(Entity a, Entity b)
    {
        if (ReferenceEquals(a, null) && ReferenceEquals(b, null))
            return true;
 
        if (ReferenceEquals(a, null) || ReferenceEquals(b, null))
            return false;
 
        return a.Equals(b);
    }
 
    public static bool operator !=(Entity a, Entity b)
    {
        return !(a == b);
    }
 
    public override int GetHashCode()
    {
        return (GetRealType().ToString() + Id).GetHashCode();
    }
 
    public virtual bool IsTransient()
    {
        return Id == 0;
    }
 
    public virtual Type GetRealType()
    {
        return NHibernateUtil.GetClass(this);
    }
}

Every Entity class inherits from it.

It is a slight variation of the code here: https://github.com/vkhorikov/DddInAction/blob/master/DddInPractice.Logic/Common/Entity.cs

I am trying to decide whether to remove this base entity type from my application because of the feedback I have received when talking about it.

Is it normal to have a base Entity type (and base ValueObject) that has a base implementation for comparisons e.g. Object.Equals, IEquality.Equals etc?

It seems logical to me. Say I have the following entities:

Customer
Supplier
Product
Production
etc

Why is it a bad idea for them all to be compared by ID by default?

I have a separate domain model and data model if that has any bearing.

Answer 1

Review

The following methods are candidates for properties. A property is expected to return fast, without too much calculations and internal flow. I'm assuming NHibernateUtil.GetClass provides a fast lookup.

public virtual bool IsTransient()
{
    return Id == 0;
}

public virtual Type GetRealType()
{
    return NHibernateUtil.GetClass(this);
}

public virtual bool IsTransient => Id == 0;
public virtual Type RealType => NHibernateUtil.GetClass(this);

---

When to use a base class

• Since C# allows for single class inheritance, pick a base class that adds sufficient state / operations to the class. Providing base classes like EqualityBase, ComparableBase, DisposableBase, HashCodeBase are bad choices, since they don't provide meaningful context except for a very specific mitigation of boiler-plate code. Which class should Entity implement of the above? You would like to implement all of them, but you can't!
• Is Entity provided by the OP a good choice for a base class? If the IsTransient() and GetRealType() methods weren't provided, I would argue against using a base class. You would end up with a class like above (EqualityBase, HashCodeBase mix). Both added methods allow for sufficient context to justify a base class.
• Use the power of interface inheritance, because you can implement as many interfaces as you want, and composition, because you can contain and delegate operations to as many contained objects you desire.

---

Dilemma

Using a base class allows for DRY code. The repetitive boiler-plate code sits in the base class, while the entities are very clean POCO's. However, you force the entities to derive from this base class, limiting the inheritance options for such classes drastically.

Solution

To allow for flexibility in inheritance options for your entities, and mitigating most boiler-plate code, you could provide an interface, a base class and let your entities implement both. The special entities that require to inherit from a different base class, should do so, and implement the interface.

public interface IEntity
{
    long Id { get; }
    bool IsTransient { get; }
    Type RealType { get; }
} 

public abstract class EntityBase : IEntity
{
    public virtual long Id { get; protected set; }
    public virtual bool IsTransient => Id == 0;
    public virtual Type RealType => NHibernateUtil.GetClass(this);

    // .. other code (equals, operator overloads, ..)
}

Most entities would implement both the base class and interface:

public class Employee : EntityBase
{
    public string Name { get; set; }
    public string FirstName { get; set; }
    public string BadgeId { get; set; }
}

Allowing for special cases:

public class DocumentElement : TreeNode<DocumentElement>, IEntity
{
    // .. DocumentElement  members
    // .. IEntity members
}

Other layers using the entities don't care about the base class, only the interface.

public interface IRepository<T> where T : IEntity
{
    T Get(int id);
    void Save(T entity);
}

Answer 2

WTF

I'm imagining someone must have read about potential quirks coming from the Delta Quadrant of the C# universe and decided a top to bottom rewrite of equality behavior is needed, just to be safe. Yeah, those guys on the C# design and compiler team got nuttin' on me!

Re-implementing inherited behavior in an override is what is going on here. This is antithetical, contrary, to the fundamental point of OO inheritance.

Overriding == does give nice, concise expression but essentially everything else I see is WTF.

All that is required is a straight-forward Equals implementation so your custom class, collections, and comparisons in general, work as intended. The only gotcha I've ever covered (consciously on purpose) was when I had an IEqualityComparer and made sure to override Equals to also call it.

The question makes no sense

All c# objects have a base and inherit Equals, and always, ultimately, from Object. Bottom line, the Equals implementation must differentiate objects such that your collections, hash sets, etc. work as expected.

The implementation makes no sense

The default, inherited, Object.Equals implementation is ReferenceEquals.

A key point of inheritance is that base behavior does not need to be, should not be, duplicated in an override.

ReferenceEquals(.., null) - I'm sure there's some obscure point to this but in all the thousands of Equals overrides I've seen and written, this was not there. Just check for if (other == null) return false;. And the fact that null is not an object per-se makes ReferenceEquals look odd and quirky. NOTE: I've seen buggy IComparer / CompareTo where null caused an infinite loop but the problem there was coder incompetence and poor testing, not lack of ReferenceEquals in that class' Equals override implementation.