Implementing equals & hashCode methods
------=_Part_6882949_1833668210.1401021211136 Content-Type: text/plain; charset=utf-8 Content-Transfer-Encoding: 7bit Hi All, Recently I reviewed a patch that adds a new business entity to the engine. The entity class has the following members: * id * data center id * name * type * some other properties that do not belong to the entity key The equals & hashCode methods were implemented in a way that include all members. I asked the patch author to change that, so it'll include only business key (data center id, name and type), which define the entity uniqueness. Also I found that many other business entities are implemented in a similar way (include all class members in equals & hashCode). I'm a new to oVirt, so I'd like to ask your opinion on the issue. 1. Do you agree with my approach on equals & hashCode. 2. If you agree with my approach in general, should we implement it in the new introduced code or should we adhere to the old convention even we do not agree with it? 3. Should we re-factor the old code (it might be dangerous as we do have enough unit test coverage)? Thanks in advance, Yevgeny ------=_Part_6882949_1833668210.1401021211136 Content-Type: text/html; charset=utf-8 Content-Transfer-Encoding: quoted-printable <html><body><div style=3D"font-family: times new roman, new york, times, se= rif; font-size: 12pt; color: #000000"><div>Hi All,<br></div><div><br></div>= <div>Recently I reviewed a patch that adds a new business entity to the eng= ine.<br>The entity class has the following members:<br><ul><li>id</li><li>d= ata center id</li><li>name<br></li><li>type</li><li>some other properties t= hat do not belong to the entity key</li></ul>The equals & hashCode meth= ods were implemented in a way that include all members.<br></div><div>I ask= ed the patch author to change that, so it'll include only business key (dat= a center id, name and type), which define the entity uniqueness.<br></div><= div>Also I found that many other business entities are implemented in a sim= ilar way (include all class members in equals & hashCode).<br></div><di= v><br></div><div>I'm a new to oVirt, so I'd like to ask your opinion on the= issue. </div><div><ol><li>Do you agree with my approach on equals & ha= shCode.</li><li>If you agree with my approach in general, should we impleme= nt it in the new introduced code or should we adhere to the old convention = even we do not agree with it?<br></li><li>Should we re-factor the old code = (it might be dangerous as we do have enough unit test coverage)?<br></li></= ol><br></div><div>Thanks in advance,<br></div><div>Yevgeny<br></div></div><= /body></html> ------=_Part_6882949_1833668210.1401021211136--
----- Original Message -----
From: "Yevgeny Zaspitsky" <yzaspits@redhat.com> To: devel@ovirt.org Sent: Sunday, May 25, 2014 3:33:31 PM Subject: [ovirt-devel] Implementing equals & hashCode methods
Hi All,
Recently I reviewed a patch that adds a new business entity to the engine. The entity class has the following members:
* id * data center id * name * type * some other properties that do not belong to the entity key
The equals & hashCode methods were implemented in a way that include all members. I asked the patch author to change that, so it'll include only business key (data center id, name and type), which define the entity uniqueness. Also I found that many other business entities are implemented in a similar way (include all class members in equals & hashCode).
I'm a new to oVirt, so I'd like to ask your opinion on the issue.
1. Do you agree with my approach on equals & hashCode. 2. If you agree with my approach in general, should we implement it in the new introduced code or should we adhere to the old convention even we do not agree with it? 3. Should we re-factor the old code (it might be dangerous as we do have enough unit test coverage)?
Thanks in advance, Yevgeny
I assume your idea came (at least from ) the way identify is defined at Hibernate. The advantages are obvious - a. shorter code b. let's time to compute However, I fear this is may lead to some bugs if identities are not defined well. Regarding 3 - might be dangerous as we don't have enough unit test coverage, probably. Actually, IMHO the situation with DAO tests which uses equals is quite good. For hashCode - I assume you have a point
_______________________________________________ Devel mailing list Devel@ovirt.org http://lists.ovirt.org/mailman/listinfo/devel
See my comments in your mail body. ----- Original Message -----
From: "Yair Zaslavsky" <yzaslavs@redhat.com> To: "Yevgeny Zaspitsky" <yzaspits@redhat.com> Cc: devel@ovirt.org Sent: Sunday, May 25, 2014 4:09:11 PM Subject: Re: [ovirt-devel] Implementing equals & hashCode methods
----- Original Message -----
From: "Yevgeny Zaspitsky" <yzaspits@redhat.com> To: devel@ovirt.org Sent: Sunday, May 25, 2014 3:33:31 PM Subject: [ovirt-devel] Implementing equals & hashCode methods
Hi All,
Recently I reviewed a patch that adds a new business entity to the engine. The entity class has the following members:
* id * data center id * name * type * some other properties that do not belong to the entity key
The equals & hashCode methods were implemented in a way that include all members. I asked the patch author to change that, so it'll include only business key (data center id, name and type), which define the entity uniqueness. Also I found that many other business entities are implemented in a similar way (include all class members in equals & hashCode).
I'm a new to oVirt, so I'd like to ask your opinion on the issue.
1. Do you agree with my approach on equals & hashCode. 2. If you agree with my approach in general, should we implement it in the new introduced code or should we adhere to the old convention even we do not agree with it? 3. Should we re-factor the old code (it might be dangerous as we do have enough unit test coverage)?
Thanks in advance, Yevgeny
I assume your idea came (at least from ) the way identify is defined at Hibernate. The advantages are obvious - a. shorter code b. let's time to compute
However, I fear this is may lead to some bugs if identities are not defined well.
We should strive to define our classes well. Actually that point refers to the design stage rather than to the implementation. On other hand, if we put the objects (that include all members in their equals) in a HashSet in order to keep unique instances, we might have a bug there.
Regarding 3 - might be dangerous as we don't have enough unit test coverage, probably. Actually, IMHO the situation with DAO tests which uses equals is quite good.
Those classes are used all over engine code and even in the UI code, so changing the implementation in the existing code is very risky. Having DAO covered isn't enough IMHO.
For hashCode - I assume you have a point
As you probably know hashCode implementation could not be (conceptually) different from the equals one.
_______________________________________________ Devel mailing list Devel@ovirt.org http://lists.ovirt.org/mailman/listinfo/devel
Thanks, Gilad. ----- Original Message -----
From: "Yevgeny Zaspitsky" <yzaspits@redhat.com> To: "Yair Zaslavsky" <yzaslavs@redhat.com> Cc: devel@ovirt.org Sent: Sunday, May 25, 2014 4:52:30 PM Subject: Re: [ovirt-devel] Implementing equals & hashCode methods
See my comments in your mail body.
----- Original Message -----
From: "Yair Zaslavsky" <yzaslavs@redhat.com> To: "Yevgeny Zaspitsky" <yzaspits@redhat.com> Cc: devel@ovirt.org Sent: Sunday, May 25, 2014 4:09:11 PM Subject: Re: [ovirt-devel] Implementing equals & hashCode methods
----- Original Message -----
From: "Yevgeny Zaspitsky" <yzaspits@redhat.com> To: devel@ovirt.org Sent: Sunday, May 25, 2014 3:33:31 PM Subject: [ovirt-devel] Implementing equals & hashCode methods
Hi All,
Recently I reviewed a patch that adds a new business entity to the engine. The entity class has the following members:
* id * data center id * name * type * some other properties that do not belong to the entity key
The equals & hashCode methods were implemented in a way that include all members. I asked the patch author to change that, so it'll include only business key (data center id, name and type), which define the entity uniqueness. Also I found that many other business entities are implemented in a similar way (include all class members in equals & hashCode).
I'm a new to oVirt, so I'd like to ask your opinion on the issue.
1. Do you agree with my approach on equals & hashCode. 2. If you agree with my approach in general, should we implement it in the new introduced code or should we adhere to the old convention even we do not agree with it? 3. Should we re-factor the old code (it might be dangerous as we do have enough unit test coverage)?
I agree it is a very important issue, but since oVirt 3.5 is a right at the door, I suggest to postpone this discussion to quieter times :-) You are more than welcome to submit a fix - it'd most appreciated. Thanks, Gilad.
Thanks in advance, Yevgeny
I assume your idea came (at least from ) the way identify is defined at Hibernate. The advantages are obvious - a. shorter code b. let's time to compute
However, I fear this is may lead to some bugs if identities are not defined well.
We should strive to define our classes well. Actually that point refers to the design stage rather than to the implementation.
On other hand, if we put the objects (that include all members in their equals) in a HashSet in order to keep unique instances, we might have a bug there.
Regarding 3 - might be dangerous as we don't have enough unit test coverage, probably. Actually, IMHO the situation with DAO tests which uses equals is quite good.
Those classes are used all over engine code and even in the UI code, so changing the implementation in the existing code is very risky. Having DAO covered isn't enough IMHO.
For hashCode - I assume you have a point
As you probably know hashCode implementation could not be (conceptually) different from the equals one.
_______________________________________________ Devel mailing list Devel@ovirt.org http://lists.ovirt.org/mailman/listinfo/devel
_______________________________________________ Devel mailing list Devel@ovirt.org http://lists.ovirt.org/mailman/listinfo/devel
See my comments in your mail body. ----- Original Message -----
From: "Gilad Chaplik" <gchaplik@redhat.com> To: "Yevgeny Zaspitsky" <yzaspits@redhat.com> Cc: "Yair Zaslavsky" <yzaslavs@redhat.com>, devel@ovirt.org Sent: Sunday, May 25, 2014 8:41:36 PM Subject: Re: [ovirt-devel] Implementing equals & hashCode methods
Thanks, Gilad.
----- Original Message -----
From: "Yevgeny Zaspitsky" <yzaspits@redhat.com> To: "Yair Zaslavsky" <yzaslavs@redhat.com> Cc: devel@ovirt.org Sent: Sunday, May 25, 2014 4:52:30 PM Subject: Re: [ovirt-devel] Implementing equals & hashCode methods
See my comments in your mail body.
----- Original Message -----
From: "Yair Zaslavsky" <yzaslavs@redhat.com> To: "Yevgeny Zaspitsky" <yzaspits@redhat.com> Cc: devel@ovirt.org Sent: Sunday, May 25, 2014 4:09:11 PM Subject: Re: [ovirt-devel] Implementing equals & hashCode methods
----- Original Message -----
From: "Yevgeny Zaspitsky" <yzaspits@redhat.com> To: devel@ovirt.org Sent: Sunday, May 25, 2014 3:33:31 PM Subject: [ovirt-devel] Implementing equals & hashCode methods
Hi All,
Recently I reviewed a patch that adds a new business entity to the engine. The entity class has the following members:
* id * data center id * name * type * some other properties that do not belong to the entity key
The equals & hashCode methods were implemented in a way that include all members. I asked the patch author to change that, so it'll include only business key (data center id, name and type), which define the entity uniqueness. Also I found that many other business entities are implemented in a similar way (include all class members in equals & hashCode).
I'm a new to oVirt, so I'd like to ask your opinion on the issue.
1. Do you agree with my approach on equals & hashCode. 2. If you agree with my approach in general, should we implement it in the new introduced code or should we adhere to the old convention even we do not agree with it? 3. Should we re-factor the old code (it might be dangerous as we do have enough unit test coverage)?
I agree it is a very important issue, but since oVirt 3.5 is a right at the door, I suggest to postpone this discussion to quieter times :-) You are more than welcome to submit a fix - it'd most appreciated.
Gilad, thank you for opinion even though it's not very relevant to the thread.
Thanks, Gilad.
Thanks in advance, Yevgeny
I assume your idea came (at least from ) the way identify is defined at Hibernate. The advantages are obvious - a. shorter code b. let's time to compute
However, I fear this is may lead to some bugs if identities are not defined well.
We should strive to define our classes well. Actually that point refers to the design stage rather than to the implementation.
On other hand, if we put the objects (that include all members in their equals) in a HashSet in order to keep unique instances, we might have a bug there.
Regarding 3 - might be dangerous as we don't have enough unit test coverage, probably. Actually, IMHO the situation with DAO tests which uses equals is quite good.
Those classes are used all over engine code and even in the UI code, so changing the implementation in the existing code is very risky. Having DAO covered isn't enough IMHO.
For hashCode - I assume you have a point
As you probably know hashCode implementation could not be (conceptually) different from the equals one.
_______________________________________________ Devel mailing list Devel@ovirt.org http://lists.ovirt.org/mailman/listinfo/devel
_______________________________________________ Devel mailing list Devel@ovirt.org http://lists.ovirt.org/mailman/listinfo/devel
----- Original Message -----
From: "Yevgeny Zaspitsky" <yzaspits@redhat.com> To: "Gilad Chaplik" <gchaplik@redhat.com> Cc: devel@ovirt.org Sent: Monday, May 26, 2014 10:25:08 AM Subject: Re: [ovirt-devel] Implementing equals & hashCode methods
See my comments in your mail body.
----- Original Message -----
From: "Gilad Chaplik" <gchaplik@redhat.com> To: "Yevgeny Zaspitsky" <yzaspits@redhat.com> Cc: "Yair Zaslavsky" <yzaslavs@redhat.com>, devel@ovirt.org Sent: Sunday, May 25, 2014 8:41:36 PM Subject: Re: [ovirt-devel] Implementing equals & hashCode methods
Thanks, Gilad.
----- Original Message -----
From: "Yevgeny Zaspitsky" <yzaspits@redhat.com> To: "Yair Zaslavsky" <yzaslavs@redhat.com> Cc: devel@ovirt.org Sent: Sunday, May 25, 2014 4:52:30 PM Subject: Re: [ovirt-devel] Implementing equals & hashCode methods
See my comments in your mail body.
----- Original Message -----
From: "Yair Zaslavsky" <yzaslavs@redhat.com> To: "Yevgeny Zaspitsky" <yzaspits@redhat.com> Cc: devel@ovirt.org Sent: Sunday, May 25, 2014 4:09:11 PM Subject: Re: [ovirt-devel] Implementing equals & hashCode methods
----- Original Message -----
From: "Yevgeny Zaspitsky" <yzaspits@redhat.com> To: devel@ovirt.org Sent: Sunday, May 25, 2014 3:33:31 PM Subject: [ovirt-devel] Implementing equals & hashCode methods
Hi All,
Recently I reviewed a patch that adds a new business entity to the engine. The entity class has the following members:
* id * data center id * name * type * some other properties that do not belong to the entity key
The equals & hashCode methods were implemented in a way that include all members. I asked the patch author to change that, so it'll include only business key (data center id, name and type), which define the entity uniqueness. Also I found that many other business entities are implemented in a similar way (include all class members in equals & hashCode).
I'm a new to oVirt, so I'd like to ask your opinion on the issue.
1. Do you agree with my approach on equals & hashCode.
Yes, when we have a single UUID/Long key (see my comment in 3)
2. If you agree with my approach in general, should we implement it in the new introduced code or should we adhere to the old convention even we do not agree with it?
Change it when applicable
3. Should we re-factor the old code (it might be dangerous as we do have enough unit test coverage)?
Yes, we just have to avoid that when the key is not a single column UUID/long and it is actually a collection of columns (i.e. columnA,columnB for example) since this key may be subjected to a change (for example adding another column to the key)
I agree it is a very important issue, but since oVirt 3.5 is a right at the door, I suggest to postpone this discussion to quieter times :-) You are more than welcome to submit a fix - it'd most appreciated.
Gilad, thank you for opinion even though it's not very relevant to the thread.
Thanks, Gilad.
Thanks in advance, Yevgeny
I assume your idea came (at least from ) the way identify is defined at Hibernate. The advantages are obvious - a. shorter code b. let's time to compute
However, I fear this is may lead to some bugs if identities are not defined well.
We should strive to define our classes well. Actually that point refers to the design stage rather than to the implementation.
On other hand, if we put the objects (that include all members in their equals) in a HashSet in order to keep unique instances, we might have a bug there.
Regarding 3 - might be dangerous as we don't have enough unit test coverage, probably. Actually, IMHO the situation with DAO tests which uses equals is quite good.
Those classes are used all over engine code and even in the UI code, so changing the implementation in the existing code is very risky. Having DAO covered isn't enough IMHO.
For hashCode - I assume you have a point
As you probably know hashCode implementation could not be (conceptually) different from the equals one.
_______________________________________________ Devel mailing list Devel@ovirt.org http://lists.ovirt.org/mailman/listinfo/devel
_______________________________________________ Devel mailing list Devel@ovirt.org http://lists.ovirt.org/mailman/listinfo/devel
_______________________________________________ Devel mailing list Devel@ovirt.org http://lists.ovirt.org/mailman/listinfo/devel
I'm also new to oVirt, so I can just add mine opinion to strengthen what you've said. What you've described is a recommended way how to form "identity" in hibernate, that's true. Use 'candidate key' instead of just comparing upon PK — some business/natural key of that entity, not to be dependent on entity persistent state. The only problem is, and I encounter it countless times, that there can be a lot of situations, where I cannot find such natural key, or I don't want to persist properties of real-world object, forming that natural key. Lets say I want to persist comments to an article, then natural key could be precise timestamp & identification of author. In app without authentization I don't want to store such data along with comments. (If you know solution to this, let me know.) In this case, most common thing I saw is to just compare ID & do not compare unpersisted entities, or comparing every property, which is failproof I think. To your question: ad 1. I totally agree with your approach, where applicable. ad 2. I don't see any benefit in sticking to "everywhere in same way" approach, it goes against evolution, and just brings stability in suboptimal position. Almost every convention (except for code-style) is wrong. Every convention requiring something in uniform, duplicate manner lets say "name all queries with suffix '_query'" should be expressed programmatically to avoid errors. In our case if we want 'all members equals/hashcode for business objects' then parent for all business objects with reflection final equals/hashcode should be introduced to eliminate possible errors. (note: this is rather problematic with gwt, but if I checked correctly, maybe doable). ad 3. I cannot see any benefit in batch changing working code, which does not influence performance is not required. If product owner is ok with this to change, is ok to change it (after lot of tests), when some business objects is 'touched' by some of your changes. Even if you potentially break it(and you can never completely avoid it), it's just one thing that got broken. m. ----- Original Message ----- From: "Yair Zaslavsky" <yzaslavs@redhat.com> To: "Yevgeny Zaspitsky" <yzaspits@redhat.com> Cc: devel@ovirt.org Sent: Sunday, May 25, 2014 3:09:11 PM Subject: Re: [ovirt-devel] Implementing equals & hashCode methods ----- Original Message -----
From: "Yevgeny Zaspitsky" <yzaspits@redhat.com> To: devel@ovirt.org Sent: Sunday, May 25, 2014 3:33:31 PM Subject: [ovirt-devel] Implementing equals & hashCode methods
Hi All,
Recently I reviewed a patch that adds a new business entity to the engine. The entity class has the following members:
* id * data center id * name * type * some other properties that do not belong to the entity key
The equals & hashCode methods were implemented in a way that include all members. I asked the patch author to change that, so it'll include only business key (data center id, name and type), which define the entity uniqueness. Also I found that many other business entities are implemented in a similar way (include all class members in equals & hashCode).
I'm a new to oVirt, so I'd like to ask your opinion on the issue.
1. Do you agree with my approach on equals & hashCode. 2. If you agree with my approach in general, should we implement it in the new introduced code or should we adhere to the old convention even we do not agree with it? 3. Should we re-factor the old code (it might be dangerous as we do have enough unit test coverage)?
Thanks in advance, Yevgeny
I assume your idea came (at least from ) the way identify is defined at Hibernate. The advantages are obvious - a. shorter code b. let's time to compute However, I fear this is may lead to some bugs if identities are not defined well. Regarding 3 - might be dangerous as we don't have enough unit test coverage, probably. Actually, IMHO the situation with DAO tests which uses equals is quite good. For hashCode - I assume you have a point
_______________________________________________ Devel mailing list Devel@ovirt.org http://lists.ovirt.org/mailman/listinfo/devel
_______________________________________________ Devel mailing list Devel@ovirt.org http://lists.ovirt.org/mailman/listinfo/devel
Sorry, seems like I missed NOT in (3). :-( So my opinion on the test coverage is NOT enough, which makes re-factoring dangerous. I hope that will make the question clearer. Yevgeny ----- Original Message -----
From: "Yevgeny Zaspitsky" <yzaspits@redhat.com> To: devel@ovirt.org Sent: Sunday, May 25, 2014 3:33:31 PM Subject: Implementing equals & hashCode methods
Hi All,
Recently I reviewed a patch that adds a new business entity to the engine. The entity class has the following members:
* id * data center id * name * type * some other properties that do not belong to the entity key
The equals & hashCode methods were implemented in a way that include all members. I asked the patch author to change that, so it'll include only business key (data center id, name and type), which define the entity uniqueness. Also I found that many other business entities are implemented in a similar way (include all class members in equals & hashCode).
I'm a new to oVirt, so I'd like to ask your opinion on the issue.
1. Do you agree with my approach on equals & hashCode. 2. If you agree with my approach in general, should we implement it in the new introduced code or should we adhere to the old convention even we do not agree with it? 3. Should we re-factor the old code (it might be dangerous as we do have enough unit test coverage)?
Thanks in advance, Yevgeny
participants (5)
-
Eli Mesika -
Gilad Chaplik -
Martin Mucha -
Yair Zaslavsky -
Yevgeny Zaspitsky