Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Rewrite / check our equals/hashcode methods #1504

Open
4 tasks done
sbernard31 opened this issue Sep 1, 2023 · 40 comments
Open
4 tasks done

Rewrite / check our equals/hashcode methods #1504

sbernard31 opened this issue Sep 1, 2023 · 40 comments
Labels
housekeeping Refactoring, cleaning code or API

Comments

@sbernard31
Copy link
Contributor

sbernard31 commented Sep 1, 2023

Investigating on NPE in LwM2mPath.hashcode() find at #1502.

I decide to add unit tests to check the bug before to fix it.

I found that library to do that job : EqualsVerifier.

Good news : it does a very good job.
Bad news: this is more complicate than expected to implement good custom equals/hashcode method.

We should :

  1. Have a look at all custom equals/hashcode methods and add a corresponding test based on EqualsVerifier.
  2. Decide some convention about written equals/hashcode in Leshan.
  3. Reimplement equals/hashcode if needed.
  4. Add documentation about that in : https://github.com/eclipse-leshan/leshan/wiki/Code-&-design-guidelines#code-rules

Let's use this issue to talk about "Decide some convention about written equals/hashcode in Leshan".
If you interested about that you should first read : How to Write an Equality Method in Java

My current opinion :

  • we should probably use Objects.equals and Objects.hash

  • Use of this 3 a valid behavior :
    1. Use classic getClass() != obj.getClass() + making the class final : this is the simple way but I do not like too much the idea I prefer to let user being able to extend class.
    2. Use an instanceof + make hashcode/equals final : this is simple enough and this allow to extend the class but not to customize equals behavior...
    3. Use the canEqual method, which is a bit unusual but sounds to solve all problems.

    Should we encourage to use 3. in all code base ?

@nkrzykala
Copy link
Contributor

nkrzykala commented Jul 29, 2024

Hello, I'm a new internship student at Orange under the lead of @JaroslawLegierski.

I was reading about this issue and I added test to almost every class which overrides equals and hashCode using EqualsVerifier (locally for now, not sure if this is needed).

Then I tried to implement canEqual method in the LwM2mPath class as @sbernard31 suggested, with the help of this article.

This is what I have now:

@Override
public final boolean equals(Object obj) {
        boolean result = false;
        if (obj instanceof LwM2mPath) {
            LwM2mPath that = (LwM2mPath) obj;
            result = (that.canEqual(this) && Objects.equals(objectId, that.objectId) && Objects.equals(objectInstanceId, that.objectInstanceId)
                    && Objects.equals(resourceId, that.resourceId)
                    && Objects.equals(resourceInstanceId, that.resourceInstanceId));
        }
        return result;
    }

public boolean canEqual(Object obj) {
       return (obj instanceof LwM2mPath);
    }

Is this the implementation you thought of in the comment?

The EqualsVerifier test passes without .simple(), but now there is another problem. I get a warning saying that equals and hashCode methods should be final (see the warning here). Is this a big challange for us?

Other tests showed some different warnings. I believe I could go into more detail once you find time to look again at this issue. Thanks

@sbernard31
Copy link
Contributor Author

Is this the implementation you thought of in #1504 (comment)?

Yep that's pretty much the idea. if we decide to go with 3.
I still don't know what is the best way to follow. I really don't like 1.
But for most of the case maybe 2. is enough ?

At Orange do you have any opinion about that ?
Maybe we go with 2. and when we find use case where inheritance is needed with implement 3. ?

I was reading about this issue and I added test to almost every class which overrides equals and hashCode using EqualsVerifier

@nkrzykala how many class did you find ? the number could help to answer to question above ☝️

The EqualsVerifier test passes without .simple(), but now there is another problem. I get a warning saying that equals and hashCode methods should be final (see the warning here). Is this a big challange for us?

If we go with 2. no need to solve this (as we will have equals and hashcode final)
If we go with 3. we probably need to write something like :

    private class ExtendedLwM2mPath extends LwM2mPath {
        public ExtendedLwM2mPath(int objectId) throws InvalidLwM2mPathException {
            super(objectId);
        }
        @Override
        public boolean canEqual(Object obj) {
            return (obj instanceof ExtendedLwM2mPath);
        }
    }

    @Test
    public void assertEqualsHashcode() {
        EqualsVerifier.forClass(LwM2mPath.class).withRedefinedSubclass(ExtendedLwM2mPath.class).verify();
    }

@nkrzykala
Copy link
Contributor

I found 67 classes which override equals and hashCode.

We agree that your approach:

Maybe we go with 2. and when we find use case where inheritance is needed with implement 3.

sounds good.

@sbernard31
Copy link
Contributor Author

I found 67 classes which override equals and hashCode.

Are there classes which extend or are extended ?

Maybe we go with 2. and when we find use case where inheritance is needed with implement 3.
sounds good.

OK so let's go for 2. (instanceof + final equals/hashcode) for most classes and if there is some classes which are extended lets using 3. (canEqual).

Just to let you know, with eclipse you can generate equals using Source / Generate hascode() and equals()..., then you can check :
Capture d’écran du 2024-07-30 11-10-11

This gives this kind of code :

    @Override
    public int hashCode() {
        return Objects.hash(objectId, objectInstanceId, resourceId, resourceInstanceId);
    }

    @Override
    public boolean equals(Object obj) {
        if (this == obj)
            return true;
        if (!(obj instanceof LwM2mPath))
            return false;
        LwM2mPath other = (LwM2mPath) obj;
        return Objects.equals(objectId, other.objectId) && Objects.equals(objectInstanceId, other.objectInstanceId)
                && Objects.equals(resourceId, other.resourceId)
                && Objects.equals(resourceInstanceId, other.resourceInstanceId);
    }

Then you just need to add final to both methods.

If you are not using eclipse, please try to produce code which looks like this 🙏

@nkrzykala
Copy link
Contributor

nkrzykala commented Aug 8, 2024

@sbernard31 I found 68 classes which override equals and hashCode, 20-25 of them are extended or extend (or both).

I have other questions:

1. In some cases the EqualsVerifier tests I wrote (in clasess which extend other class) return an error saying that not every field is used in equals/hashCode.
For example I have generated equals & hashCode and added canEqual for WriteRequest class (it extends AbstractSimpleDownlinkRequest):

@Override
    public boolean equals(Object o) {
        if (!(o instanceof WriteRequest)) return false;
        if (!super.equals(o)) return false;
        WriteRequest that = (WriteRequest) o;
        return that.canEqual(this) && Objects.equals(node, that.node) && Objects.equals(contentFormat, that.contentFormat) && mode == that.mode;
    }

    public boolean canEqual(Object o) {
        return (o instanceof WriteRequest);
    }

    @Override
    public int hashCode() {
        return Objects.hash(super.hashCode(), node, contentFormat, mode);
    }

The test looks like this:

    private class ExtendedWriteRequest extends WriteRequest {
        public ExtendedWriteRequest(Mode mode, ContentFormat contentFormat, int objectId, int objectInstanceId,
                            Collection<LwM2mResource> resources) throws InvalidRequestException {
            super(mode, contentFormat, newPath(objectId, objectInstanceId), new LwM2mObjectInstance(objectId, resources),
                    null);
        }
        @Override
        public boolean canEqual(Object obj) {
            return (obj instanceof ExtendedWriteRequest);
        }
    }

    @Test
    public void assertEqualsHashcode() {
        EqualsVerifier.forClass(WriteRequest.class).withRedefinedSuperclass().withRedefinedSubclass(ExtendedWriteRequest.class).verify();
    }

I get an error that "coapRequest" field is not included in equals or it is stateless (see this).
coapRequest field is in the AbstractLwM2mRequest class which AbstractSimpleDownlinkRequest class extends (same problem for example in BootstrapRequest class which extends AbstractLwM2mRequest class directly).
Should I ommit this field with .withIgnoredFields() or is there an error in my implementation?

2. In some other tests I get an error saying that fields of type BigDecimal should be compared using compareTo() in instead of Objects.equals(). Can we do that or should we suppress this warning? see this
3. In class RouterService there is a transient field (isPrefixed) used in equals and hashCode. Should we delete this field from equals&hashCode or suppress this warning? see this
4. What if the fields are not final? see this

Thanks for advice

@sbernard31
Copy link
Contributor Author

1. coapRequest field should not be included in equals. So yes should should omit this field.

2. Yep we should use compareTo as we probably want to compare by value.

3. This is more or less a copy/paste from java-coap, It is not really necessary to test this one. I should be removed to use the java-coap one at mid term. If you really want to test it just remove the transient keyword. I don't understand why transient it used, so I asked to java-coap : open-coap/java-coap#88

4. this smell not so good, I guess we should study each case 1 by 1.

@nkrzykala
Copy link
Contributor

2. I tried to add compareTo as it was showed here:

    @Override
    public final boolean equals(Object o) {
        if (this == o) return true;
        if (!(o instanceof JsonArrayEntry)) return false;
        JsonArrayEntry that = (JsonArrayEntry) o;
        boolean comparablyEqual = (time == null && that.time == null)
                || (time != null && that.time != null && time.compareTo(that.time) == 0);
        return Objects.equals(name, that.name) && Objects.equals(floatValue, that.floatValue)
                && Objects.equals(booleanValue, that.booleanValue) && Objects.equals(objectLinkValue, that.objectLinkValue)
                && Objects.equals(stringValue, that.stringValue) && comparablyEqual;
    }

Also modified the hashCode to use .stripTrailingZeros() so that the hashcode is consistent. Then, I got the NPE error in hashCode, on the field "time" (the one that is BigDecimal), see this. I guess that I should check in hashCode if the field is NULL. Is this possible?:

    @Override
    public final int hashCode() {
        int result = Objects.hash(name, floatValue, booleanValue, objectLinkValue, stringValue);
        result = 31 * result + (time != null ? time.stripTrailingZeros().hashCode() : 0);
        return result;
    }

4. The cases of the not final fields error are in classes:

  • 4.1. SenMLRecord - all fields are not final (9)
  • 4.2. JsonArrayEntry - all fields are not final (6)
  • 4.3. JsonRootObject - all fields are not final (3)
  • 4.4. MixedLwM2mLink - all fields are not final (2)
  • 4.5. BaseAttribute - 2 out of 3 fields are not final
  • 4.6. ValuelessAttribute - all fields are not final (1)
  • 4.7. DeregisterRequest - all fields are not final (1)
  • 4.8. SenMLPack - all fields are not final (1)

@sbernard31
Copy link
Contributor Author

see this.

The this link is not good or ?

I tried to add compareTo as it was showed here:

Maybe we should consider ""Normalise the BigDecimal fields during your class’s construction" ?
I will look at this next week.

4. The cases of the not final fields error are in classes:

For :

  • 4.4. MixedLwM2mLink - all fields are not final (2)
  • 4.5. BaseAttribute - 2 out of 3 fields are not final
  • 4.6. ValuelessAttribute - all fields are not final (1)
  • 4.7. DeregisterRequest - all fields are not final (1)

I think make the field final should do the trick.

For :

  • 4.8. SenMLPack - all fields are not final (1)
  • 4.1. SenMLRecord - all fields are not final (9)
  • 4.2. JsonArrayEntry - all fields are not final (6)
  • 4.3. JsonRootObject - all fields are not final (3)

Use .suppress(Warning.NONFINAL_FIELDS) then we will fix that in another PR later.

@nkrzykala
Copy link
Contributor

nkrzykala commented Aug 12, 2024

The this link is not good or ?

Sorry, I meant the site about NPE error.

Maybe we should consider ""Normalise the BigDecimal fields during your class’s construction" ?

OK, I will also look at this.

I think make the field final should do the trick

I'm afraid it won't work for DeregisterRequest class because there's a field registrationId which cannot be final (one of the constructors assigns a value to this field). Should I add the .suppress(Warning.NONFINAL_FIELDS) here as well?

Also I found another class with not final fields - Tlv

Thanks for the answer

@sbernard31
Copy link
Contributor Author

I'm afraid it won't work for DeregisterRequest class because there's a field registrationId which cannot be final (one of the constructors assigns a value to this field).

I think we can just remove the null initialization in attribute definition.

---    private String registrationId = null;
+++    private final String registrationId;

@sbernard31
Copy link
Contributor Author

Also I found another class with not final fields - Tlv

Check if setters are used in code (just by removing it and see if code still compiles)
If yes, I think the better solution is to remove setter and make the fields final.

@sbernard31
Copy link
Contributor Author

sbernard31 commented Aug 12, 2024

About "Normalise the BigDecimal fields during your class’s construction", that comes with usage of Warning.BIGDECIMAL_EQUALITY but this is surprising to me so I ask about it : jqno/equalsverifier#987

@nkrzykala
Copy link
Contributor

As I can see, open-coap/java-coap#52 (comment) says that I should delete the transient field from equals&hashCode.

Also in jqno/equalsverifier#987 what I understand:
it is either compareTo() or you have to use Warning.BIGDECIMAL_EQUALITY because you can't really test the normalisation of BigDecimal fields...

If you find time I would like to ask you to look into classes:

  1. StringUtils - there is an equals() method but not the overriden one
  2. SenMLTestUtil - same, there is an equals() method but not the overriden one

I don't know how to treat them. They don't look like they should be tested by the EqualsVerifier because they have a different purpose?

  1. ULong - I added canEqual method (because ULong extends Number), but it doesn't change anything because ULong is final... should I leave ULong without canEqual?

  2. LwM2mResourceInstance - there are comments in hashCode() and equals(): "custom hashcode/equals to handle arrays". Should I leave the custom as it is or the generated hashCode&equals methods are OK for you too?

@sbernard31
Copy link
Contributor Author

As I can see, open-coap/java-coap#52 (comment) says that I should delete the transient field from equals&hashCode.

Nope just delete the transient key word, we duplicate that class especially because prefix in not in equals, see : open-coap/java-coap#52 (comment)

Also in jqno/equalsverifier#987 what I understand:
it is either compareTo() or you have to use Warning.BIGDECIMAL_EQUALITY

For now I understand that too.

If we go with "normalisation" (and so we use Warning.BIGDECIMAL_EQUALITY) maybe we can complete test of BigDecimal with a manual test like this : jqno/equalsverifier#987 (comment)

because you can't really test the normalisation of BigDecimal fields...

Yep not possible for now in EqualsVerifier, maybe in the future : jqno/equalsverifier#987 (comment)

So we must decide compareTo() or normalization at constructor ?
First, come at cost of custom equals/hashcode() and make equals and hashcode slower when used.
Second, come at cost of suppress warning + manual test, and make constructor slower.

If you find time I would like to ask you to look into classes:

StringUtils - there is an equals() method but not the overriden one
SenMLTestUtil - same, there is an equals() method but not the overriden one

this is static method this is out of scope of that PR, let this aside for now. (unless you think you maybe find an issue with it ? )

ULong - I added canEqual method (because ULong extends Number), but it doesn't change anything because ULong is final... should I leave ULong without canEqual?

The class is final so no need to add canEqual, as I understand that canEqual is to support inheritance.

LwM2mResourceInstance - there are comments in hashCode() and equals(): "custom hashcode/equals to handle arrays". Should I leave the custom as it is or the generated hashCode&equals methods are OK for you too?

Oh I looked at this class and I'm not sure EqualsVerifier will really like it ... (jqno/equalsverifier#987 (comment))
This is typically case where there is a kind of logic between attribute value...
But we need to have custom hashcode/equals for Array because of https://stackoverflow.com/questions/8777257/equals-vs-arrays-equals-in-java/8777266#8777266

@nkrzykala
Copy link
Contributor

Second, come at cost of suppress warning + manual test, and make constructor slower.

Plus we need to add .stripTrailingZeros() to setters, which may also make them slower?

I have no strong opinion on whether compareTo() or normalisation is better/more efficient... I don't know what is more important for Leshan.

But we need to have custom hashcode/equals for Array because of https://stackoverflow.com/questions/8777257/equals-vs-arrays-equals-in-java/8777266#8777266

OK, I understand. I tried to test with EqualsVerifier on the original, custom equals&hashCode and I got this error:
https://jqno.nl/equalsverifier/errormessages/subclass-object-is-not-equal-to-an-instance-of-a-trivial-subclass-with-equal-fields/
One of the proposed solutions there is using instanceof so maybe we could do in equals:

-        if (obj == null)
-            return false;
-        if (getClass() != obj.getClass())
-            return false;
+       if (!(obj instanceof LwM2mResourceInstance)) return false;

and leave the custom rest. After that there is only symmetry problem which can be resolved by making the methods final (the class isn't extended and doesn't extend other class).

@sbernard31
Copy link
Contributor Author

I have no strong opinion on whether compareTo() or normalisation is better/more efficient... I don't know what is more important for Leshan.

🤔 my guess most of the class will not be too much used in a Map but constructor is always called.
So maybe better to take performance penalty only on equals/hashcode in a first time, and so go for compateTo

So I suppose we go back to your initial question : #1504 (comment)

and you could do something like this :

public static class ClassWithBigDecimalField {
    private final BigDecimal bigdecimalField;
    private final String stringfield;

    public ClassWithBigDecimalField(BigDecimal b, String s) {
        this.bigdecimalField = b;
        this.stringfield = s;
    }

    @Override
    public final boolean equals(Object obj) {
        if (this == obj)
            return true;
        if (!(obj instanceof ClassWithBigDecimalField))
            return false;
        ClassWithBigDecimalField other = (ClassWithBigDecimalField) obj;

        boolean comparablyEqual = (bigdecimalField == null && other.bigdecimalField == null)
                || (bigdecimalField != null && other.bigdecimalField != null
                        && bigdecimalField.compareTo(other.bigdecimalField) == 0);

        return comparablyEqual && Objects.equals(stringfield, other.stringfield);
    }

    @Override
    public final int hashCode() {
        return Objects.hash(bigdecimalField != null ? bigdecimalField.stripTrailingZeros() : null, stringfield);
    }

    @Override
    public String toString() {
        return String.format("ClassWithBigDecimalField [bigdecimalField=%s, stringfield=%s]", bigdecimalField,
                stringfield);
    }
}

One of the proposed solutions there is using instanceof so maybe we could do in equals:

Yep using instanceof + final equals/hashcode is what we decide above : #1504 (comment)

@nkrzykala
Copy link
Contributor

and you could do something like this

OK, sure

Yep using instanceof + final equals/hashcode is what we decide above: #1504 (comment)

Yes, I wanted to make sure if this could be done here as well, without changing the custom parts 🙂

@nkrzykala
Copy link
Contributor

Hello, I have pushed all changes to my branch: https://github.com/JaroslawLegierski/leshan/tree/opl/natalia_test

I'm waiting for your review😄

@sbernard31
Copy link
Contributor Author

Could you create a PR this will be easier to review it 🙂

@nkrzykala
Copy link
Contributor

Hi, PR #1644 is now created.

I'll be gone from the office next week, but ready on the 2nd of September to do fixups!

@sbernard31
Copy link
Contributor Author

sbernard31 commented Sep 23, 2024

So #1644 is now integrated in master.

Remaining works are :

@nkrzykala Let me know if you plan to work on that ? (1. or 2. or both)
(Just to know for organization 🙂)

@nkrzykala
Copy link
Contributor

Yes, I can work on that, preferably on the 1.

@sbernard31
Copy link
Contributor Author

Yes, I can work on that, preferably on the 1.

Not a surprise . Programmer rarely like writing documentation 😁

So do you want hint for that 4 classes ? or you want to try to propose your own solution based on your knowledge on this topic ?

  • SenMLPack works with SenMLRecord,
  • And JsonArrayEntry works with JsonRootObject.

So better to separate works/discussion in 2 tasks. (no preferred order for me)

@sbernard31
Copy link
Contributor Author

Sure, should I create issues or would you like to do that?

We can start the discussion here and continue it in 2 separated PRs but you can also create dedicate issue if you prefer.

(any preferable way to name them for recognition?)

If you create an issue, feel free to choose a name.

So do you want hint for that 4 classes ?

OK so following #1504 (comment), we decide to
" go for 2. (instanceof + final equals/hashcode) for most classes and if there is some classes which are extended lets using 3. (canEqual). "

Here no need to extend, so we go with (instanceof + final equals/hashcode).
But we still have the Non Final Field Warning which is not so good because :

"This means that if the value of this field changes, the same object may not be equal to itself at two different points in time. This can be a problem if you use this object as a key in a map, or if you put it in a HashSet or other hash-based collection: the collection might not be able to find your object again."

So we should probably change that class to make it immutable by :

  • simple solution remove setter and just keep constructor.
  • if needed, eventually add a Builder.

Does it sounds good to you ?

@sbernard31
Copy link
Contributor Author

sbernard31 commented Sep 24, 2024

I forget to mention that to make a class immutable. Attributes should generally be final but they should be immutable too.
If an attribute is a collection, that collection should be immutable too (or at least unmodifiable).

Since Java 9 there is immutable collection. But we target java 8 as minimal java version so we can not use it and so we need to fall back with unmodifiable collection. (see Collections.unmodifiableList)

@nkrzykala
Copy link
Contributor

We can start the discussion here and continue it in 2 separated PRs but you can also create dedicate issue if you prefer.

Ok, let's stay here and go for 2 PRs later on.

simple solution remove setter

I thought about that at first when I got the error of non final fields (I made them final and realised that setters are problematic then). But then I asked for your opinion right away and didn't check if the setters are needed, now I now they are used.

and just keep constructor

Keep or add? I see only no or default constructors.

@nkrzykala
Copy link
Contributor

Since Java 9 there is immutable collection. But we target java 8 as minimal java version so we can not use it and so we need to fall back with unmodifiable collection. (see Collections.unmodifiableList)

OK, I didn't know that, I'll look at this

@sbernard31
Copy link
Contributor Author

sbernard31 commented Sep 24, 2024

Keep or add? I see only no or default constructors.

I didn't check the code but "keep if already exist" OR "add if needed"

now I know they are used.

Yep so we need to rewrite code which use them.

@sbernard31
Copy link
Contributor Author

(@nkrzykala I integrated lot of PR recently do not forget to checkout last version of master before to work)

@nkrzykala
Copy link
Contributor

Hi, I have a question. For example in JsonArrayEntry - I added final to all fields and removed setters. Then I added a simple constructor:

    public JsonArrayEntry() {
        this.name = null;
        this.floatValue = null;
        this.booleanValue = null;
        this.objectLinkValue = null;
        this.stringValue = null;
        this.time = null;
    }

& default constructor (it was required in JsonSerializerTest). The setters I deleted were used in JsonArrayEntrySerDes. How to replace them? For all I know there are two options:

1. Use the constructor, something like:

// Extracting fields
    String name = null;
    BigDecimal time = null;
    Number floatValue = null;
    Boolean booleanValue = null;
    String stringValue = null;
    String objectLinkValue = null;

    // Extracting "name"
    JsonNode n = o.get("n");
    if (n != null && n.isTextual()) {
        name = n.asText();
    }

    // Extracting "time"
    JsonNode t = o.get("t");
    if (t != null && t.isNumber()) {
        time = new BigDecimal(t.asText());
    }
...
// Use constructor with the created values
    JsonArrayEntry jae = new JsonArrayEntry(name, floatValue, booleanValue, objectLinkValue, stringValue, time);

2. Use builder - create it and then something like:

JsonArrayEntry.Builder builder = new JsonArrayEntry.Builder();

    // Extract "name"
    JsonNode n = o.get("n");
    if (n != null && n.isTextual()) {
        builder.setName(n.asText());
    }

    // Extract "time"
    JsonNode t = o.get("t");
    if (t != null && t.isNumber()) {
        builder.setTime(new BigDecimal(t.asText()));
    }
...

I know you said:

if needed, eventually add a Builder

and my question is if you consider Builder needed here? In my opinion it's not, but I'm sure you have a valuable opinion on that.

@sbernard31
Copy link
Contributor Author

Good question.

I said "if needed" but I guess a Builder is rarely (never?) really "needed", this is more about "convenience". (so very subjective)

For this example, I agree with you adding a builder sounds overkill.

For LwM2mNodeJsonEncoder where JsonArrayEntry is also used, maybe this will be harder to do without Builder but If you find an elegant way without Builder. It's OK for me.

@JaroslawLegierski
Copy link
Contributor

I would like to finish this task started by Natalia. Please let me know if these changes are in line with what you expect?

@sbernard31
Copy link
Contributor Author

@JaroslawLegierski yep that's the idea.
I guess that if you finish that work that means that @nkrzykala does not plan to work on Leshan at short term anymore, right ?

@JaroslawLegierski
Copy link
Contributor

I finished refactoring 4 last classes

I don't know if following description will be OK as documentation in Code & design guidelines?:

Every class should be tested using EqualsVerifier. In Leshan, this ensures that the equals and hashCode implementations comply with their respective contracts. EqualsVerifier is a library designed specifically for thoroughly testing these methods.
To meet these requirements:

  • hashCode and equals methods must be declared as final to prevent unexpected behavior in subclasses.
  • The equals method should use the instanceof operator to verify the object's type and utilize equals for field-by-field comparison.
  • For classes designed to be extended, the canEqual method should be implemented to properly handle equality checks across hierarchies.

@sbernard31
Copy link
Contributor Author

sbernard31 commented Nov 27, 2024

About description, that sounds good, maybe a little addition :

Every class with custom equals/hashcode(or something like this) should be tested using EqualsVerifier. In Leshan, this ensures that the equals and hashCode implementations comply with their respective contracts. EqualsVerifier is a library designed specifically for thoroughly testing these methods.
To meet these requirements:

  • hashCode and equals methods must be declared as final to prevent unexpected behavior in subclasses.
  • The equals method should use the instanceof operator to verify the object's type and utilize equals for field-by-field comparison.
  • For classes designed to be extended, the canEqual method should be implemented to properly handle equality checks across hierarchies.

@sbernard31
Copy link
Contributor Author

I finished refactoring 4 last classes

Let's open a PR. (Do not forget to rebase on master)

@JaroslawLegierski
Copy link
Contributor

OK PR created in #1678

@JaroslawLegierski
Copy link
Contributor

About description, that sounds good, maybe a little addition :

Every class with custom equals/hashcode(or something like this) should be tested using EqualsVerifier. In Leshan, this ensures that the equals and hashCode implementations comply with their respective contracts. EqualsVerifier is a library designed specifically for thoroughly testing these methods.
To meet these requirements:

  • hashCode and equals methods must be declared as final to prevent unexpected behavior in subclasses.
  • The equals method should use the instanceof operator to verify the object's type and utilize equals for field-by-field comparison.
  • For classes designed to be extended, the canEqual method should be implemented to properly handle equality checks across hierarchies.

I modified Code & design guidelines by adding this description in Code rules chapter

@sbernard31
Copy link
Contributor Author

Thx @JaroslawLegierski.

I try to move forward on vue3 migration #1665 and then I need to take a look at #1671 quickly, so not sure I will review the PR very soon. I hope it's ok for you.

If you think this should be done quickly, let me know and I will try to find time :)

@JaroslawLegierski
Copy link
Contributor

JaroslawLegierski commented Nov 29, 2024

If you think this should be done quickly, let me know and I will try to find time :)

OK There is no urgency on our side.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
housekeeping Refactoring, cleaning code or API
Projects
None yet
Development

No branches or pull requests

3 participants