[ovirt-devel] Implementing equals & hashCode methods

[Eli Mesika]

----- Original Message -----
> From: "Yevgeny Zaspitsky" <yzaspits at redhat.com>
> To: "Gilad Chaplik" <gchaplik at redhat.com>
> Cc: devel at 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 at redhat.com>
> > To: "Yevgeny Zaspitsky" <yzaspits at redhat.com>
> > Cc: "Yair Zaslavsky" <yzaslavs at redhat.com>, devel at 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 at redhat.com>
> > > To: "Yair Zaslavsky" <yzaslavs at redhat.com>
> > > Cc: devel at 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 at redhat.com>
> > > > To: "Yevgeny Zaspitsky" <yzaspits at redhat.com>
> > > > Cc: devel at 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 at redhat.com>
> > > > > To: devel at 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 at ovirt.org
> > > > > http://lists.ovirt.org/mailman/listinfo/devel
> > > > 
> > > _______________________________________________
> > > Devel mailing list
> > > Devel at ovirt.org
> > > http://lists.ovirt.org/mailman/listinfo/devel
> > > 
> > 
> _______________________________________________
> Devel mailing list
> Devel at ovirt.org
> http://lists.ovirt.org/mailman/listinfo/devel
>