object == object instead of object.id == object.id potential problems

Question

I have inherited a very sloppy project and I am tasked with explaining why its bad. I've noticed all over the code they have done comparisons like this

(IQueryable).FirstOrDefault(x => x.Facility == facility && x.Carrier == shipCode.Carrier

in the example above x.Facility is from the database and facility is from shipment.facility which is mapped as a complex object in nhibernate.

I would have expected to see .FirstOrDefault(x => x.Facility.ID == facility.ID

At first I thought comparing the whole object might cause issues if the facility record was changed in the db then the facility stored in shipment would obviously be different.

After thinking about it more I realized that shipment.facility was populated from the id so it should match even if that facility record changed.

It still feels wrong to me I see this being very buggy and hard to track down? Is there somthing specifically wrong with comparing the entire object vs an id.

Answer 1

Expanding my previous comments into an answer:

I think this is actually good practice when the ORM allows it. I'm not experienced with NHibernate, so for the rest of the answer I'll assume that it does in fact implement equality in a sensible way (such as comparing primary keys). You should ensure this is the case, otherwise the code would be not only bad but potentially buggy.

As an analogy, forget about SQL for the moment, imagine you were dealing with a POCO which was not part of any ORM. You want to choose between the following:

Approach 1: IEquatable

public class Facility : IEquatable<Facility>
{
    public int Id {get; private set;}
    //The rest of the properties

    public bool Equals(Facility other)
    {
        return other.Id == Id;
    }
}

(You'd also want to override Object.Equals, but I'll exclude that for brevity)

Approach 2: IEqualityComparer

public class Facility
{
    public int Id {get; private set;}
    //The rest of the properties
}

public class FacilityIdsMatchEqualityComparer : IEqualityComparer<Facility>
{
    public bool Equals(Facility x, Facility y)
    {
        return x.Id == y.Id;
    }
}

(GetHashCode also excluded for brevity).

Which of the two approaches is better? Well, I'd say it's Approach 1 by a clear margin. It adheres to two important principles that Approach 2 doesn't:

• Don't repeat yourself. In the second approach, any code anywhere trying to compare facilities would have to use the FacilityIdsMatchEqualityComparer. This fact, that particular equality comparison logic needs to be used, would be sprayed all over the solution, repeated every time you want to compare them.

• Single responsibility principle. Not only does every class doing the comparison need to repeat code, but that code is taking on a responsibility that doesn't belong to the class. It should be up to the Facility to express the implementation of equality, not up to every class that wants to use it to say that it's done by comparing Ids. (Yes I realise that you could just make it, say, FacilityEqualityComparer so that the calling classes remain agnostic about how the equality comparison is done, but the purpose of this is as an analogy with the code in the OP, where the exact comparison logic is hard coded into every comparison)

So, bringing it back to the actual question, this analogy very closely mirrors the situation you have here. It's not quite as simple as the Facility implementing IEquatable, but the principle is exactly the same: the ORM is taking its own responsibility for how equality checking is implemented, rather than pushing that responsibility out to code using it.

This means that if I'm writing code, say outside of the data access layer, I can say "I want to check if these two objects are equal, so I'll write object1 == object2", rather than "I want to check if these two objects are equal, and these two objects are entities, which because of the way we implement our persistence means that when I check for equality that will be converted into a SQL query, so I need to write this check as if I were writing SQL, which means I need to compare their primary keys, which either by checking attributes or maybe through my knowledge of conventions in the data access layer, I know means comparing their Id properties. So I'll write object1.Id == object2.Id".

ORMs aren't perfect, and you can't always completely abstract away the underlying SQL database, but when you can, you should!