-
Notifications
You must be signed in to change notification settings - Fork 409
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
Comments
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:
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 |
Yep that's pretty much the idea. if we decide to go with 3. At Orange do you have any opinion about that ?
@nkrzykala how many class did you find ? the number could help to answer to question above ☝️
If we go with 2. no need to solve this (as we will have equals and hashcode final) 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();
} |
I found 67 classes which override equals and hashCode. We agree that your approach:
sounds good. |
@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.
The test looks like this:
I get an error that "coapRequest" field is not included in equals or it is stateless (see this). 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 Thanks for advice |
1. 2. Yep we should use 3. This is more or less a copy/paste from 4. this smell not so good, I guess we should study each case 1 by 1. |
2. I tried to add compareTo as it was showed here:
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?:
4. The cases of the not final fields error are in classes:
|
The
Maybe we should consider ""Normalise the BigDecimal fields during your class’s construction" ?
For :
I think make the field final should do the trick. For :
Use |
Sorry, I meant the site about NPE error.
OK, I will also look at this.
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 |
I think we can just remove the null initialization in attribute definition. --- private String registrationId = null;
+++ private final String registrationId; |
Check if setters are used in code (just by removing it and see if code still compiles) |
About "Normalise the BigDecimal fields during your class’s construction", that comes with usage of |
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: If you find time I would like to ask you to look into classes:
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?
|
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)
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)
Yep not possible for now in EqualsVerifier, maybe in the future : jqno/equalsverifier#987 (comment) So we must decide
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 ? )
The class is final so no need to add canEqual, as I understand that canEqual is to support inheritance.
Oh I looked at this class and I'm not sure EqualsVerifier will really like it ... (jqno/equalsverifier#987 (comment)) |
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.
OK, I understand. I tried to test with EqualsVerifier on the original, custom equals&hashCode and I got this error: - 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). |
🤔 my guess most of the class will not be too much used in a Map but constructor is always called. 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);
}
}
Yep using |
OK, sure
Yes, I wanted to make sure if this could be done here as well, without changing the custom parts 🙂 |
Hello, I have pushed all changes to my branch: https://github.com/JaroslawLegierski/leshan/tree/opl/natalia_test I'm waiting for your review😄 |
Could you create a PR this will be easier to review it 🙂 |
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! |
So #1644 is now integrated in Remaining works are :
@nkrzykala Let me know if you plan to work on that ? (1. or 2. or both) |
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 ?
So better to separate works/discussion in 2 tasks. (no preferred order for me) |
We can start the discussion here and continue it in 2 separated PRs but you can also create dedicate issue if you prefer.
If you create an issue, feel free to choose a name.
OK so following #1504 (comment), we decide to Here no need to extend, so we go with (instanceof + final equals/hashcode). "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 :
Does it sounds good to you ? |
I forget to mention that to make a class immutable. Attributes should generally be final but they should be immutable too. 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 |
Ok, let's stay here and go for 2 PRs later on.
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.
Keep or add? I see only no or default constructors. |
OK, I didn't know that, I'll look at this |
I didn't check the code but "keep if already exist" OR "add if needed"
Yep so we need to rewrite code which use them. |
(@nkrzykala I integrated lot of PR recently do not forget to checkout last version of |
Hi, I have a question. For example in JsonArrayEntry - I added final to all fields and removed setters. Then I added a simple constructor:
& 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:
2. Use builder - create it and then something like:
I know you said:
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. |
Good question. I said "if needed" but I guess a For this example, I agree with you adding a builder sounds overkill. For |
I would like to finish this task started by Natalia. Please let me know if these changes are in line with what you expect? |
@JaroslawLegierski yep that's the idea. |
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.
|
About description, that sounds good, maybe a little addition :
|
Let's open a PR. (Do not forget to rebase on |
OK PR created in #1678 |
I modified Code & design guidelines by adding this description in Code rules chapter |
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 :) |
OK There is no urgency on our side. |
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 :
equals
/hashcode
methods and add a corresponding test based on EqualsVerifier.equals
/hashcode
in Leshan.equals
/hashcode
if needed.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
andObjects.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 ?
The text was updated successfully, but these errors were encountered: