-1

I am, specifically concerned with obeying the symmetry part of the general contract established in Object#equals(Object) where, given two non-null objects x and y, the result of x.equals(y) and y.equals(x) should be the same.

Suppose you have two classes, PurchaseOrder and InternationalPurchaseOrder where the latter extends the former. In the basic case, it makes sense that comparing an instance of each against the other shall return consistently false for x.equals(y) and y.equals(x) simply because a PurchaseOrder is not always an InternationalPurchaseOrder and therefore, the additional fields in InternationalPurchaseOrder objects will not be present in instances of PurchaseOrder.

Now, suppose you upcast an instance of InternationalPurchaseOrder.

PurchaseOrder order1 = new PurchaseOrder();
PurchaseOrder order2 = new InternationalPurchaseOrder();
System.out.println("Order 1 equals Order 2? " + order1.equals(order2));
System.out.println("Order 2 equals Order 1? " + order2.equals(order1));

We already established that the result should be symmetric. But, should the result be false for cases when both object contain the same internal data? I believe the result should be true regardless of the fact that one object has an extra field. Since by upcasting order2, access to fields in InternationalPurchaseOrder class is restricted, the result of the equals() method shall be the result of the call to the super.equals(obj).

If all I stated is true, the implementation of the equals method in InternationalPurchaseOrder should be something like this:

@Override
public boolean equals(Object obj) {
    
     if (!super.equals(obj)) return false;
     
     // PurchaseOrder already passed check by calling super.equals() and this object is upcasted
     InternationalPurchaseOrder other = (InternationalPurchaseOrder)obj;
     
        if (this.country == null) {
            if (other.country != null) return false;
        } else if (!country.equals(other.country)) return false;

        return true;
}

Assuming that country is the only field declared in this subclass.

The problem is in the super-class

@Override
public boolean equals (Object obj) {
    if (this == obj) return true;
    if (obj == null) return false;
    if (getClass() != obj.getClass()) return false;
    PurchaseOrder other = (PurchaseOrder) obj;
    if (description == null) {
        if (other.description != null) return false;
    } else if (!description.equals(other.description)) return false;
    if (orderId == null) {
        if (other.orderId != null) return false;
    } else if (!orderId.equals(other.orderId)) return false;
    if (qty != other.qty) return false;
    return true;
}

Because the two instances are not of the same class. And if instead I use getClass().isAssignableFrom(Class), symmetry is lost. The same is true when using instanceof. In the book, Effective Java, Joshua Bloch indirectly warn about overriding equals unnecessarily. In this case, however, it is necessary for the subclass to have an overridden equals for instances to compare the fields declared in the subclass.

This has gone long enough. Is this a case for a more complicated implementation of the equals function, or is this simply a case against upcasting?

CLARIFICATION: This question is not answered by the suggestions proposed by @Progman in the comments section below. This is not a simple case of overriding equals methods for subclasses. I think the code I posted here shows that I have done this correctly. This post is SPECIFICALLY about the expected result of comparing two objects when one is upcasted so that it behaves like an object of the super-class.

hfontanez
  • 5,774
  • 2
  • 25
  • 37
  • Does this answer your question? [Java - equals method in base class and in subclasses](https://stackoverflow.com/questions/13162188/java-equals-method-in-base-class-and-in-subclasses) – Progman Mar 07 '21 at 20:14
  • Also check https://stackoverflow.com/questions/596462/any-reason-to-prefer-getclass-over-instanceof-when-generating-equals – Progman Mar 07 '21 at 20:15
  • @Progman neither of those answer this question. – hfontanez Mar 07 '21 at 20:20
  • @user15244370 I think I know that and I believe I stated that in the post. The question is specifically what should be the result of comparing two objects when one is downcasted. – hfontanez Mar 07 '21 at 20:22
  • 1
    Keep in mind that "downcasting" an object doesn't change it's type. The variable `order2` is still referencing an `InternationalPurchaseOrder` object and `order2.equals(order1)` is calling the `equals()` method from the `InternationalPurchaseOrder` class. Which makes the `equals()` method return `false` as of your sentence "[...] shall return consistently `false` for `x.equals(y)` and `y.equals(x)` simply because a `PurchaseOrder` is not always an `InternationalPurchaseOrder` and therefore [...]". How are you planning to return `true` when you also say it should return `false`? – Progman Mar 07 '21 at 20:51
  • @Progman that's the philosophical point I am raising. I think the answer is that downcasting is a code smell (for the issue raised here) and that, instead of downcast, the object should be adapted (converted) and not downcasted. – hfontanez Mar 07 '21 at 21:13

2 Answers2

1

I think the misconception here is that upcasting actually changes the type of an object.

Any type of casting is just a pointer to the compiler to treat a variable as having a certain type, for the developers benefit. So upcasting a variable with type InternationalPurchaseOrder to a PurchaseOrder might change the way you see it and can interact with it (as you mentioned, fields being restricted), but the variable still references an InternationalPurchaseOrder

Therefore to concretely answer your question, comparing classes will use the equals method declared to their actual type, not the type you have marked it as.

Henry Twist
  • 5,666
  • 3
  • 19
  • 44
  • Right... my point of conflict is that, I can see why so many people consider downcasting a code smell. This might prove that point. On the other hand, when you downcast, you do it for the purpose of masking data in the sublcass. For example downcasting a long to an int. It is as if the purpose is to ignore the upper bytes and just consider the lower bytes. So, if that is true, shouldn't comparing `Long` and `Integer` yield `true` if the values of the lower bytes are the same? Because that is not possible, the only alternative I see is use an Adapter to convert and not downcast. – hfontanez Mar 07 '21 at 21:10
  • I still think this poses an interesting philosophical point. – hfontanez Mar 07 '21 at 21:11
  • 1
    @hfontanez: It sounds like you're talking about *upcasting* rather than downcasting. (It's best not to use `int` and `long` for this, given that they're primitives.) Casting something from `Object` to `String` is downcasting; casting from `String` to `Object` is upcasting. The line `PurchaseOrder order2 = new InternationalPurchaseOrder();` is an implicit upcast, from `InternationalPurchaseOrder` to `PurchaseOrder`. – Jon Skeet Mar 08 '21 at 06:48
  • @JonSkeet thanks for the correction. I should edit the post with that change. I am still inclined to believe that this does more harm than good and that it is indeed a code smell. Instead, the real solution is to use Dependency Injection and use the technique I illustrated in my posted answer. It effectively does what I thought is the right thing to do: Take the "base" set of data and compare the two objects. Using the `asPurchaseOrder` method allows me to do just that. Prefer composition over inheritance!!! – hfontanez Mar 08 '21 at 07:24
1

It turns out, the problem is a bit deeper than I originally thought for one main reason: Liskov Subsitution Principle.

My PurchaseOrder#equals(Object) method violates LSP. Because InternationalPurchaseOrder extends PurchaseOrder, it is correct to say that an international purchase IS-A purchase order. Therefore, to comply with LSP, I should be able to replace one instance for the other without any ill effects. Because of this, the line if (getClass() != obj.getClass()) return false; completely violates this principle.

Even when my focus was around upcasting (which by now I am pretty much convinced that it is a code smell), almost the same issue applies: Should I be able to compare instances of PurchaseOrder and InternationalPurchaseOrder and return true if they contain the same data internally? According to Joshua Bloch book Effective Java, Item 10 (Obey the general contract when overriding equals), the answer should be yes, but it is indeed no; but not for the reasons I initially thought (they are not the same type). The problem is this, and I quote:

There is no way to extend an instantiable class and add value component while preserving the equals contract, unless you're willing to forgo the benefits of object-oriented abstraction.

He continues to get in more details about this. The conclusion is simple: Do it this way and loose the benefits of abstraction, and violate LSP in the process. Or... simply follow a very important programming principle (especially in Java): favor composition over inheritance and inject the attributes you need to make a PurchaseOrder domestic or international in this example. In my case, this solution will look like this (irrelevant details of class omitted):

public class InternationalPurchaseOrder {
    private PurchaseOrder po;
    private String country;

    public (PurchaseOrder po, String country) {
        this.po = po;
        this.country = country;
    }

    public PurchaseOrder asPurchaseOrder() {
        return po;
    }

    @Override
    public boolean equals (Object obj) {
        if (!(obj instanceof InternationalPurchaseOrder)) {
            return false;
        }

        InternationalPurchaseOrder ipo = (InternationalPurchaseOrder)obj;
        return ipo.po.equals(po) && cp.country.equals(country);
    }
}

This way, an InternationalPurchaseOrdercan continue to be compared to other instances of the same class AND if needed to be compared to objects of PurchaseOrder type, all that is needed is to call the asPurchaseOrder to return an object of the compatible type.

hfontanez
  • 5,774
  • 2
  • 25
  • 37