From yzaspits at redhat.com Sun May 25 08:33:31 2014 Content-Type: multipart/mixed; boundary="===============1421458023670104496==" MIME-Version: 1.0 From: Yevgeny Zaspitsky To: devel at ovirt.org Subject: [ovirt-devel] Implementing equals & hashCode methods Date: Sun, 25 May 2014 08:33:31 -0400 Message-ID: <2071271020.6882950.1401021211138.JavaMail.zimbra@redhat.com> In-Reply-To: 744032381.6879500.1401018873883.JavaMail.zimbra@redhat.com --===============1421458023670104496== Content-Type: text/plain; charset="utf-8" MIME-Version: 1.0 Content-Transfer-Encoding: quoted-printable ------=3D_Part_6882949_1833668210.1401021211136 Content-Type: text/plain; charset=3Dutf-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 me= mbers. = 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 d= o not agree with it? = 3. Should we re-factor the old code (it might be dangerous as we do hav= e enough unit test coverage)? = Thanks in advance, = Yevgeny = ------=3D_Part_6882949_1833668210.1401021211136 Content-Type: text/html; charset=3Dutf-8 Content-Transfer-Encoding: quoted-printable

Hi All,

= =3D

Recently I reviewed a patch that adds a new business entity to the eng= =3D ine.
The entity class has the following members:

id
d= =3D ata center id
name
type
some other properties t= =3D hat do not belong to the entity key

The equals & hashCode meth= =3D ods were implemented in a way that include all members.

I ask= =3D ed the patch author to change that, so it'll include only business key (dat= =3D a center id, name and type), which define the entity uniqueness.

<= =3D div>Also I found that many other business entities are implemented in a sim= =3D ilar way (include all class members in equals & hashCode).

I'm a new to oVirt, so I'd like to ask your opinion on the= =3D issue.

Do you agree with my approach on equals & ha= =3D shCode.
If you agree with my approach in general, should we impleme= =3D nt it in the new introduced code or should we adhere to the old convention = =3D even we do not agree with it?
Should we re-factor the old code = =3D (it might be dangerous as we do have enough unit test coverage)?

Thanks in advance,

Yevgeny

<= =3D /body> ------=3D_Part_6882949_1833668210.1401021211136-- --===============1421458023670104496== Content-Type: multipart/alternative MIME-Version: 1.0 Content-Transfer-Encoding: base64 Content-Disposition: attachment; filename="attachment.bin" LS0tLS0tPV9QYXJ0XzY4ODI5NDlfMTgzMzY2ODIxMC4xNDAxMDIxMjExMTM2CkNvbnRlbnQtVHlw ZTogdGV4dC9wbGFpbjsgY2hhcnNldD11dGYtOApDb250ZW50LVRyYW5zZmVyLUVuY29kaW5nOiA3 Yml0CgpIaSBBbGwsIAoKUmVjZW50bHkgSSByZXZpZXdlZCBhIHBhdGNoIHRoYXQgYWRkcyBhIG5l dyBidXNpbmVzcyBlbnRpdHkgdG8gdGhlIGVuZ2luZS4gClRoZSBlbnRpdHkgY2xhc3MgaGFzIHRo ZSBmb2xsb3dpbmcgbWVtYmVyczogCgogICAgKiBpZCAKICAgICogZGF0YSBjZW50ZXIgaWQgCiAg ICAqIG5hbWUgCiAgICAqIHR5cGUgCiAgICAqIHNvbWUgb3RoZXIgcHJvcGVydGllcyB0aGF0IGRv IG5vdCBiZWxvbmcgdG8gdGhlIGVudGl0eSBrZXkgCgpUaGUgZXF1YWxzICYgaGFzaENvZGUgbWV0 aG9kcyB3ZXJlIGltcGxlbWVudGVkIGluIGEgd2F5IHRoYXQgaW5jbHVkZSBhbGwgbWVtYmVycy4g CkkgYXNrZWQgdGhlIHBhdGNoIGF1dGhvciB0byBjaGFuZ2UgdGhhdCwgc28gaXQnbGwgaW5jbHVk ZSBvbmx5IGJ1c2luZXNzIGtleSAoZGF0YSBjZW50ZXIgaWQsIG5hbWUgYW5kIHR5cGUpLCB3aGlj aCBkZWZpbmUgdGhlIGVudGl0eSB1bmlxdWVuZXNzLiAKQWxzbyBJIGZvdW5kIHRoYXQgbWFueSBv dGhlciBidXNpbmVzcyBlbnRpdGllcyBhcmUgaW1wbGVtZW50ZWQgaW4gYSBzaW1pbGFyIHdheSAo aW5jbHVkZSBhbGwgY2xhc3MgbWVtYmVycyBpbiBlcXVhbHMgJiBoYXNoQ29kZSkuIAoKSSdtIGEg bmV3IHRvIG9WaXJ0LCBzbyBJJ2QgbGlrZSB0byBhc2sgeW91ciBvcGluaW9uIG9uIHRoZSBpc3N1 ZS4gCgoKICAgIDEuIERvIHlvdSBhZ3JlZSB3aXRoIG15IGFwcHJvYWNoIG9uIGVxdWFscyAmIGhh c2hDb2RlLiAKICAgIDIuIElmIHlvdSBhZ3JlZSB3aXRoIG15IGFwcHJvYWNoIGluIGdlbmVyYWws IHNob3VsZCB3ZSBpbXBsZW1lbnQgaXQgaW4gdGhlIG5ldyBpbnRyb2R1Y2VkIGNvZGUgb3Igc2hv dWxkIHdlIGFkaGVyZSB0byB0aGUgb2xkIGNvbnZlbnRpb24gZXZlbiB3ZSBkbyBub3QgYWdyZWUg d2l0aCBpdD8gCiAgICAzLiBTaG91bGQgd2UgcmUtZmFjdG9yIHRoZSBvbGQgY29kZSAoaXQgbWln aHQgYmUgZGFuZ2Vyb3VzIGFzIHdlIGRvIGhhdmUgZW5vdWdoIHVuaXQgdGVzdCBjb3ZlcmFnZSk/ IAoKVGhhbmtzIGluIGFkdmFuY2UsIApZZXZnZW55IAoKLS0tLS0tPV9QYXJ0XzY4ODI5NDlfMTgz MzY2ODIxMC4xNDAxMDIxMjExMTM2CkNvbnRlbnQtVHlwZTogdGV4dC9odG1sOyBjaGFyc2V0PXV0 Zi04CkNvbnRlbnQtVHJhbnNmZXItRW5jb2Rpbmc6IHF1b3RlZC1wcmludGFibGUKCjxodG1sPjxi b2R5PjxkaXYgc3R5bGU9M0QiZm9udC1mYW1pbHk6IHRpbWVzIG5ldyByb21hbiwgbmV3IHlvcmss IHRpbWVzLCBzZT0KcmlmOyBmb250LXNpemU6IDEycHQ7IGNvbG9yOiAjMDAwMDAwIj48ZGl2Pkhp IEFsbCw8YnI+PC9kaXY+PGRpdj48YnI+PC9kaXY+PQo8ZGl2PlJlY2VudGx5IEkgcmV2aWV3ZWQg YSBwYXRjaCB0aGF0IGFkZHMgYSBuZXcgYnVzaW5lc3MgZW50aXR5IHRvIHRoZSBlbmc9CmluZS48 YnI+VGhlIGVudGl0eSBjbGFzcyBoYXMgdGhlIGZvbGxvd2luZyBtZW1iZXJzOjxicj48dWw+PGxp PmlkPC9saT48bGk+ZD0KYXRhIGNlbnRlciBpZDwvbGk+PGxpPm5hbWU8YnI+PC9saT48bGk+dHlw ZTwvbGk+PGxpPnNvbWUgb3RoZXIgcHJvcGVydGllcyB0PQpoYXQgZG8gbm90IGJlbG9uZyB0byB0 aGUgZW50aXR5IGtleTwvbGk+PC91bD5UaGUgZXF1YWxzICZhbXA7IGhhc2hDb2RlIG1ldGg9Cm9k cyB3ZXJlIGltcGxlbWVudGVkIGluIGEgd2F5IHRoYXQgaW5jbHVkZSBhbGwgbWVtYmVycy48YnI+ PC9kaXY+PGRpdj5JIGFzaz0KZWQgdGhlIHBhdGNoIGF1dGhvciB0byBjaGFuZ2UgdGhhdCwgc28g aXQnbGwgaW5jbHVkZSBvbmx5IGJ1c2luZXNzIGtleSAoZGF0PQphIGNlbnRlciBpZCwgbmFtZSBh bmQgdHlwZSksIHdoaWNoIGRlZmluZSB0aGUgZW50aXR5IHVuaXF1ZW5lc3MuPGJyPjwvZGl2Pjw9 CmRpdj5BbHNvIEkgZm91bmQgdGhhdCBtYW55IG90aGVyIGJ1c2luZXNzIGVudGl0aWVzIGFyZSBp bXBsZW1lbnRlZCBpbiBhIHNpbT0KaWxhciB3YXkgKGluY2x1ZGUgYWxsIGNsYXNzIG1lbWJlcnMg aW4gZXF1YWxzICZhbXA7IGhhc2hDb2RlKS48YnI+PC9kaXY+PGRpPQp2Pjxicj48L2Rpdj48ZGl2 PkknbSBhIG5ldyB0byBvVmlydCwgc28gSSdkIGxpa2UgdG8gYXNrIHlvdXIgb3BpbmlvbiBvbiB0 aGU9CiBpc3N1ZS4gPC9kaXY+PGRpdj48b2w+PGxpPkRvIHlvdSBhZ3JlZSB3aXRoIG15IGFwcHJv YWNoIG9uIGVxdWFscyAmYW1wOyBoYT0Kc2hDb2RlLjwvbGk+PGxpPklmIHlvdSBhZ3JlZSB3aXRo IG15IGFwcHJvYWNoIGluIGdlbmVyYWwsIHNob3VsZCB3ZSBpbXBsZW1lPQpudCBpdCBpbiB0aGUg bmV3IGludHJvZHVjZWQgY29kZSBvciBzaG91bGQgd2UgYWRoZXJlIHRvIHRoZSBvbGQgY29udmVu dGlvbiA9CmV2ZW4gd2UgZG8gbm90IGFncmVlIHdpdGggaXQ/PGJyPjwvbGk+PGxpPlNob3VsZCB3 ZSByZS1mYWN0b3IgdGhlIG9sZCBjb2RlID0KKGl0IG1pZ2h0IGJlIGRhbmdlcm91cyBhcyB3ZSBk byBoYXZlIGVub3VnaCB1bml0IHRlc3QgY292ZXJhZ2UpPzxicj48L2xpPjwvPQpvbD48YnI+PC9k aXY+PGRpdj5UaGFua3MgaW4gYWR2YW5jZSw8YnI+PC9kaXY+PGRpdj5ZZXZnZW55PGJyPjwvZGl2 PjwvZGl2Pjw9Ci9ib2R5PjwvaHRtbD4KLS0tLS0tPV9QYXJ0XzY4ODI5NDlfMTgzMzY2ODIxMC4x NDAxMDIxMjExMTM2LS0K --===============1421458023670104496==-- From yzaslavs at redhat.com Sun May 25 09:09:14 2014 Content-Type: multipart/mixed; boundary="===============3334867418030404625==" MIME-Version: 1.0 From: Yair Zaslavsky To: devel at ovirt.org Subject: Re: [ovirt-devel] Implementing equals & hashCode methods Date: Sun, 25 May 2014 09:09:11 -0400 Message-ID: <1493281587.18406204.1401023351399.JavaMail.zimbra@redhat.com> In-Reply-To: 2071271020.6882950.1401021211138.JavaMail.zimbra@redhat.com --===============3334867418030404625== Content-Type: text/plain; charset="utf-8" MIME-Version: 1.0 Content-Transfer-Encoding: quoted-printable ----- Original Message ----- > From: "Yevgeny Zaspitsky" > To: devel(a)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 k= ey > (data center id, name and type), which define the entity uniqueness. > Also I found that many other business entities are implemented in a simil= ar > 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 h= ave > enough unit test coverage)? > = > Thanks in advance, > Yevgeny I assume your idea came (at least from ) the way identify is defined at Hib= ernate. 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(a)ovirt.org > http://lists.ovirt.org/mailman/listinfo/devel --===============3334867418030404625==-- From yzaspits at redhat.com Sun May 25 09:52:30 2014 Content-Type: multipart/mixed; boundary="===============7598632960914748540==" MIME-Version: 1.0 From: Yevgeny Zaspitsky To: devel at ovirt.org Subject: Re: [ovirt-devel] Implementing equals & hashCode methods Date: Sun, 25 May 2014 09:52:30 -0400 Message-ID: <1462971881.6885408.1401025950118.JavaMail.zimbra@redhat.com> In-Reply-To: 1493281587.18406204.1401023351399.JavaMail.zimbra@redhat.com --===============7598632960914748540== Content-Type: text/plain; charset="utf-8" MIME-Version: 1.0 Content-Transfer-Encoding: quoted-printable See my comments in your mail body. ----- Original Message ----- > From: "Yair Zaslavsky" > To: "Yevgeny Zaspitsky" > Cc: devel(a)ovirt.org > Sent: Sunday, May 25, 2014 4:09:11 PM > Subject: Re: [ovirt-devel] Implementing equals & hashCode methods > = > = > = > ----- Original Message ----- > > From: "Yevgeny Zaspitsky" > > To: devel(a)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 engi= ne. > > 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 sim= ilar > > 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 e= ven > > 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 defin= ed > 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 equ= als) in a HashSet in order to keep unique instances, we might have a bug th= ere. > = > Regarding 3 - might be dangerous as we don't have enough unit test covera= ge, > probably. > Actually, IMHO the situation with DAO tests which uses equals is quite go= od. Those classes are used all over engine code and even in the UI code, so cha= nging the implementation in the existing code is very risky. Having DAO cov= ered isn't enough IMHO. > For hashCode - I assume you have a point As you probably know hashCode implementation could not be (conceptually) di= fferent from the equals one. > = > = > = > > = > > _______________________________________________ > > Devel mailing list > > Devel(a)ovirt.org > > http://lists.ovirt.org/mailman/listinfo/devel >=20 --===============7598632960914748540==-- From gchaplik at redhat.com Sun May 25 13:41:37 2014 Content-Type: multipart/mixed; boundary="===============8903060929999363913==" MIME-Version: 1.0 From: Gilad Chaplik To: devel at ovirt.org Subject: Re: [ovirt-devel] Implementing equals & hashCode methods Date: Sun, 25 May 2014 13:41:36 -0400 Message-ID: <1929933026.7389936.1401039696429.JavaMail.zimbra@redhat.com> In-Reply-To: 1462971881.6885408.1401025950118.JavaMail.zimbra@redhat.com --===============8903060929999363913== Content-Type: text/plain; charset="utf-8" MIME-Version: 1.0 Content-Transfer-Encoding: quoted-printable Thanks, = Gilad. ----- Original Message ----- > From: "Yevgeny Zaspitsky" > To: "Yair Zaslavsky" > Cc: devel(a)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" > > To: "Yevgeny Zaspitsky" > > Cc: devel(a)ovirt.org > > Sent: Sunday, May 25, 2014 4:09:11 PM > > Subject: Re: [ovirt-devel] Implementing equals & hashCode methods > > = > > = > > = > > ----- Original Message ----- > > > From: "Yevgeny Zaspitsky" > > > To: devel(a)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 busine= ss > > > 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 def= ined > > 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 b= ug > 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(a)ovirt.org > > > http://lists.ovirt.org/mailman/listinfo/devel > > = > _______________________________________________ > Devel mailing list > Devel(a)ovirt.org > http://lists.ovirt.org/mailman/listinfo/devel >=20 --===============8903060929999363913==-- From mmucha at redhat.com Mon May 26 02:41:27 2014 Content-Type: multipart/mixed; boundary="===============2361077392964366848==" MIME-Version: 1.0 From: Martin Mucha To: devel at ovirt.org Subject: Re: [ovirt-devel] Implementing equals & hashCode methods Date: Mon, 26 May 2014 02:41:26 -0400 Message-ID: <1907024412.6955058.1401086486411.JavaMail.zimbra@redhat.com> In-Reply-To: 1493281587.18406204.1401023351399.JavaMail.zimbra@redhat.com --===============2361077392964366848== Content-Type: text/plain; charset="utf-8" MIME-Version: 1.0 Content-Transfer-Encoding: quoted-printable I'm also new to oVirt, so I can just add mine opinion to strengthen what yo= u've said. What you've described is a recommended way how to form "identity" in hibern= ate, that's true. Use 'candidate key' instead of just comparing upon PK =E2= =80=94 some business/natural key of that entity, not to be dependent on ent= ity persistent state. The only problem is, and I encounter it countless tim= es, 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 t= hat natural key. Lets say I want to persist comments to an article, then na= tural key could be precise timestamp & identification of author. In app wit= hout authentization I don't want to store such data along with comments. (I= f 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 compa= ring 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" appro= ach, it goes against evolution, and just brings stability in suboptimal pos= ition. Almost every convention (except for code-style) is wrong. Every conv= ention 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 o= bjects' then parent for all business objects with reflection final equals/h= ashcode should be introduced to eliminate possible errors. (note: this is r= ather problematic with gwt, but if I checked correctly, maybe doable). ad 3. I cannot see any benefit in batch changing working code, which does n= ot 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 obje= cts 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" To: "Yevgeny Zaspitsky" Cc: devel(a)ovirt.org Sent: Sunday, May 25, 2014 3:09:11 PM Subject: Re: [ovirt-devel] Implementing equals & hashCode methods ----- Original Message ----- > From: "Yevgeny Zaspitsky" > To: devel(a)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 k= ey > (data center id, name and type), which define the entity uniqueness. > Also I found that many other business entities are implemented in a simil= ar > 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 h= ave > enough unit test coverage)? > = > Thanks in advance, > Yevgeny I assume your idea came (at least from ) the way identify is defined at Hib= ernate. 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(a)ovirt.org > http://lists.ovirt.org/mailman/listinfo/devel _______________________________________________ Devel mailing list Devel(a)ovirt.org http://lists.ovirt.org/mailman/listinfo/devel --===============2361077392964366848==-- From yzaspits at redhat.com Mon May 26 03:25:09 2014 Content-Type: multipart/mixed; boundary="===============4769747679984652834==" MIME-Version: 1.0 From: Yevgeny Zaspitsky To: devel at ovirt.org Subject: Re: [ovirt-devel] Implementing equals & hashCode methods Date: Mon, 26 May 2014 03:25:08 -0400 Message-ID: <1838932822.6968893.1401089108586.JavaMail.zimbra@redhat.com> In-Reply-To: 1929933026.7389936.1401039696429.JavaMail.zimbra@redhat.com --===============4769747679984652834== Content-Type: text/plain; charset="utf-8" MIME-Version: 1.0 Content-Transfer-Encoding: quoted-printable See my comments in your mail body. ----- Original Message ----- > From: "Gilad Chaplik" > To: "Yevgeny Zaspitsky" > Cc: "Yair Zaslavsky" , devel(a)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" > > To: "Yair Zaslavsky" > > Cc: devel(a)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" > > > To: "Yevgeny Zaspitsky" > > > Cc: devel(a)ovirt.org > > > Sent: Sunday, May 25, 2014 4:09:11 PM > > > Subject: Re: [ovirt-devel] Implementing equals & hashCode methods > > > = > > > = > > > = > > > ----- Original Message ----- > > > > From: "Yevgeny Zaspitsky" > > > > To: devel(a)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 busi= ness > > > > 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 implemen= t it > > > > in > > > > the new introduced code or should we adhere to the old conventi= on > > > > even > > > > we do not agree with it? > > > > 3. Should we re-factor the old code (it might be dangerous as w= e do > > > > have > > > > enough unit test coverage)? > = > I agree it is a very important issue, but since oVirt 3.5 is a right at t= he > 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 thre= ad. > = > 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(a)ovirt.org > > > > http://lists.ovirt.org/mailman/listinfo/devel > > > = > > _______________________________________________ > > Devel mailing list > > Devel(a)ovirt.org > > http://lists.ovirt.org/mailman/listinfo/devel > > = >=20 --===============4769747679984652834==-- From emesika at redhat.com Tue May 27 04:18:47 2014 Content-Type: multipart/mixed; boundary="===============1841606294143420140==" MIME-Version: 1.0 From: Eli Mesika To: devel at ovirt.org Subject: Re: [ovirt-devel] Implementing equals & hashCode methods Date: Tue, 27 May 2014 04:18:47 -0400 Message-ID: <1523277575.10089661.1401178727086.JavaMail.zimbra@redhat.com> In-Reply-To: 1838932822.6968893.1401089108586.JavaMail.zimbra@redhat.com --===============1841606294143420140== Content-Type: text/plain; charset="utf-8" MIME-Version: 1.0 Content-Transfer-Encoding: quoted-printable ----- Original Message ----- > From: "Yevgeny Zaspitsky" > To: "Gilad Chaplik" > Cc: devel(a)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" > > To: "Yevgeny Zaspitsky" > > Cc: "Yair Zaslavsky" , devel(a)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" > > > To: "Yair Zaslavsky" > > > Cc: devel(a)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" > > > > To: "Yevgeny Zaspitsky" > > > > Cc: devel(a)ovirt.org > > > > Sent: Sunday, May 25, 2014 4:09:11 PM > > > > Subject: Re: [ovirt-devel] Implementing equals & hashCode methods > > > > = > > > > = > > > > = > > > > ----- Original Message ----- > > > > > From: "Yevgeny Zaspitsky" > > > > > To: devel(a)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 incl= ude > > > > > 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 uniquene= ss. > > > > > 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 implem= ent > > > > > it > > > > > in > > > > > the new introduced code or should we adhere to the old conven= tion > > > > > 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/lo= ng and it is actually a collection of columns (i.e. columnA,columnB for exa= mple) since this key may be subjected to a change (for example adding another col= umn 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 define= d 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 refe= rs > > > to > > > the design stage rather than to the implementation. > > > = > > > On other hand, if we put the objects (that include all members in the= ir > > > 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 qu= ite > > > > 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 (conceptual= ly) > > > different from the equals one. > > > = > > > > = > > > > = > > > > = > > > > > = > > > > > _______________________________________________ > > > > > Devel mailing list > > > > > Devel(a)ovirt.org > > > > > http://lists.ovirt.org/mailman/listinfo/devel > > > > = > > > _______________________________________________ > > > Devel mailing list > > > Devel(a)ovirt.org > > > http://lists.ovirt.org/mailman/listinfo/devel > > > = > > = > _______________________________________________ > Devel mailing list > Devel(a)ovirt.org > http://lists.ovirt.org/mailman/listinfo/devel >=20 --===============1841606294143420140==-- From yzaspits at redhat.com Tue May 27 05:13:11 2014 Content-Type: multipart/mixed; boundary="===============8703705831305902878==" MIME-Version: 1.0 From: Yevgeny Zaspitsky To: devel at ovirt.org Subject: Re: [ovirt-devel] Implementing equals & hashCode methods Date: Tue, 27 May 2014 05:13:10 -0400 Message-ID: <1432258967.7277384.1401181990877.JavaMail.zimbra@redhat.com> In-Reply-To: 2071271020.6882950.1401021211138.JavaMail.zimbra@redhat.com --===============8703705831305902878== Content-Type: text/plain; charset="utf-8" MIME-Version: 1.0 Content-Transfer-Encoding: quoted-printable 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" > To: devel(a)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 k= ey > (data center id, name and type), which define the entity uniqueness. > Also I found that many other business entities are implemented in a simil= ar > 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 h= ave > enough unit test coverage)? > = > Thanks in advance, > Yevgeny >=20 --===============8703705831305902878==--