1
\$\begingroup\$

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.

\$\endgroup\$
  • \$\begingroup\$ How is it used? \$\endgroup\$ – Mast Jan 30 '18 at 9:41
  • 1
    \$\begingroup\$ @Mast, as it stands; the only reason I use it is to add Products to a HashSet. \$\endgroup\$ – w0051977 Jan 30 '18 at 9:42
  • 1
    \$\begingroup\$ I do not fully agree with original article about this approach. Forcing a base class in languages with single inheritance is too much. You can introduce a default implementation you will use most of times but there must be an interface above. Not to mention that you're making your domain objects aware of ORM and it's even worse. Probably the point is to implement an EqualityComparer instead. \$\endgroup\$ – Adriano Repetti Jan 30 '18 at 10:23
  • \$\begingroup\$ @ Adriano Repetti, thanks. I really like the idea of creating an IEqualityComparer. How is the Domain Model aware of the ORM if there is a separate data model? \$\endgroup\$ – w0051977 Jan 30 '18 at 10:35
  • 2
    \$\begingroup\$ What's that feedback you've received that makes you doubt? \$\endgroup\$ – Alejandro Feb 18 '18 at 0:15
1
\$\begingroup\$

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);
}
\$\endgroup\$
1
\$\begingroup\$

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.

\$\endgroup\$
  • 2
    \$\begingroup\$ The question makes no sense - yup, that's why I voted to close it (over a year ago) but unfortunatelly it's still open and confuses people. WTF matches perfectly what this code ist ;-) \$\endgroup\$ – t3chb0t Aug 6 at 5:15
  • \$\begingroup\$ My main issue is this is in the wrong forum besides being "Huh??". But then I wonder if "what not to do" class of questions might be helpful - I thought "OMG! I gotta warn people, do not do this!!! - but these may too susceptible to opinionation. \$\endgroup\$ – radarbob Aug 6 at 18:23

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.