Skip to main content

Please review Issue16 "questions override equals(), but not hashCode()"

7 replies [Last post]
sergey_borodin
Offline
Joined: 2006-10-20
Points: 0

Reply viewing options

Select your preferred way to display the comments and click "Save settings" to activate your changes.
bkurotsu
Offline
Joined: 2004-12-13
Points: 0

Is that the correct svn revision URL?

sergey_borodin
Offline
Joined: 2006-10-20
Points: 0
bkurotsu
Offline
Joined: 2004-12-13
Points: 0

Well, the implementations look fine. I would note though, that they depend on the getStringValue() method to work correctly (both for equals() and hash calculation).

I approve the proposed change. But could you please investigate PropertiesQuestion to see if it will always work? For example, if new properties are added to the question in random order. Does this change the value of Properties.list(PrinterWriter)? If it is not reliable, perhaps PropertiesQuestion should be upgraded (as is noted in the source code).

sergey_borodin
Offline
Joined: 2006-10-20
Points: 0

It will not work for PropertiesQuestion, as it's getStringValue() methods returns Properties.list(), and string representations for the same maps (maps with equal key-value pairs) may be differerent (depends on order).

The solution may be is to modify getStringValue() method so that it will return string of sorted pairs. Then we could also remove PropertiesQuestion.equals() realization, as basic will be OK.

Another solution is to implement PropertiesQuestion.hashCode(), based on Properties.hashCode().

What to prefer?

--Sergey

bkurotsu
Offline
Joined: 2004-12-13
Points: 0

I think reimplementing getStringValue() sounds good. It is a method that people might actually use for something, so having it return a value which is useful/consistent seems best.

sergey_borodin
Offline
Joined: 2006-10-20
Points: 0

Ok, I've reimplemented PropertiesQuestion.getStringValue() and removed equals() implementation.

additional diffs are:
https://jtharness.dev.java.net/source/browse/jtharness?rev=888&view=rev

I've tested it a little bit, looks like it works (most important usage of equals - checking that interview wasn't changed when closing config editor).

Thanks,
Sergey

bkurotsu
Offline
Joined: 2004-12-13
Points: 0

Integrated into trunk - one change though...

When calculating the final string in PropertiesQuestion, you should use a StringBuffer instead of creating many strings with appending. I just changed that.

https://jtharness.dev.java.net/source/browse/jtharness?rev=903&view=rev