14

Some classes filled by frameworks (like beans). So you can't guaranty that all fields set.

Look to example: classes marked as @Entity usually have Integer id field. hashCode can be written as:

public int hashCode() {
    return id.hashCode();
}

but defensive code may look like:

public int hashCode() {
    return (id != null) ? id.hashCode() : 0;
}

Do I need write checks for null or surround code with try { ... } catch (Exception e) in hashCode and equals functions?

I have no arguments for defencive coding is such case because it hide putting inconsistent objects to collections and lead to late errors. Am I wrong in this position?

UPDATE I wrote such code:

import java.util.*;

class ExceptionInHashcode {

    String name;

    ExceptionInHashcode() { }
    ExceptionInHashcode(String name) { this.name = name; }

    public int hashCode() {
        // throw new IllegalStateException("xxx");
        return this.name.hashCode();
    }

    public static void main(String args[]) {
        Hashtable list = new Hashtable();
        list.put(new ExceptionInHashcode("ok"), 1);
        list.put(new ExceptionInHashcode(), 2); // fail
        System.out.println("list.size(): " + list.size());
    }
}

and run it:

java -classpath . ExceptionInHashcode
Exception in thread "main" java.lang.NullPointerException
        at ExceptionInHashcode.hashCode(ExceptionInHashcode.java:12)
        at java.util.Hashtable.hash(Hashtable.java:262)
        at java.util.Hashtable.put(Hashtable.java:547)
        at ExceptionInHashcode.main(ExceptionInHashcode.java:18)

I think that I can find error early instead of returning zero if object is in wrong state...

gavenkoa
  • 45,285
  • 19
  • 251
  • 303
  • 1
    Your hashCode implementation breaks the [`Object#hashCode`](http://docs.oracle.com/javase/6/docs/api/java/lang/Object.html#hashCode()) contract: *Whenever it is invoked on the same object more than once during an execution of a Java application, the hashCode method must consistently return the same integer, provided no information used in equals comparisons on the object is modified. This integer need not remain consistent from one execution of an application to another execution of the same application.* – Luiggi Mendoza Mar 01 '13 at 06:52
  • 5
    If you are to throw an exception I would be more inclined to throw an `IllegalStateException` rather than just let the `NullPointerException` bubble up. To me a `NullPointerException` in someone else's code smells of a bug, but an `IllegalStateException` tells me "you're not allowed to do that (yet), *by design*". – lc. Mar 01 '13 at 06:53
  • 2
    @LuiggiMendoza: Information used by the equals comparison *would* change if the id is changed though. Are you talking about the number returned, or the exception? – Jon Skeet Mar 01 '13 at 06:53
  • @JonSkeet about the number returned, since this implementation would return the `id.hashCode()` sometimes and `0` in others. – Luiggi Mendoza Mar 01 '13 at 06:54
  • @LuiggiMendoza: But so long as `id` is also used in `equals`, it doesn't violate the contract. What *exact* scenario are you considering which would break the contract, assuming that `equals` *does* use `id`? – Jon Skeet Mar 01 '13 at 06:56
  • @JonSkeet so I can have `int x = entity.hashCode(); //0... entity.setId(someClassInstance.createId()); int y = entity.hashCode; if (x != y) { System.out.println("this is fine!"); }` assuming that `someClassInstance.createId()` would return a non-null id for the entity, and this would be good? By the way, I understand it could be legal. – Luiggi Mendoza Mar 01 '13 at 06:59
  • Yes, that's fine - and explicitly covered in the contract you stated: "the hashCode method must consistently return the same integer, **provided no information used in equals comparisons on the object is modified**". The information used in equals comparisons has been modified, so it's fine for the hash code to change. – Jon Skeet Mar 01 '13 at 07:07
  • @JonSkeet what about *Whenever it is invoked **on the same object more than once** during an execution of a Java application* part before the text you pulled out? – Luiggi Mendoza Mar 01 '13 at 07:09
  • @LuiggiMendoza Assuming that `equals` uses `id` to do a comparison, by changing the `id` you have semantically changed the object to be a different one, even though the physical object in memory is the same. IMHO, the problem really comes from being able to change `id` in the first place. – lc. Mar 01 '13 at 07:10
  • @lc. so if I use this `Entity` as key for a `Map` and I change the `id` attribute value after setting the same `Entity` instance as key in the `Map`, doesn't that make the `Map` to have an odd behavior? – Luiggi Mendoza Mar 01 '13 at 07:12
  • @LuiggiMendoza Oh yeah, it does. That's why I said the problem comes from being able to change `id` after it's been set. – lc. Mar 01 '13 at 07:14
  • @lc. then this is not a valid implementation for `hashCode` =\ , it's legal but not valid. – Luiggi Mendoza Mar 01 '13 at 07:14
  • 4
    @LuiggiMendoza: Yes, it *is*. It *doesn't* violate the rules for `hashCode`. If you change the `id` after putting it into the map, *you're* violating the rules for what you do with keys. – Jon Skeet Mar 01 '13 at 07:19
  • @LuiggiMendoza: That's still covered by the "provided no information..." clause which you're completely ignoring. Basically, if you change information used by `equals` between two calls to `hashCode`, all bets are off. – Jon Skeet Mar 01 '13 at 07:21
  • @JonSkeet after searching this on the net, I found that I didn't understand the whole concept. IMO [this blog entry](http://eclipsesource.com/blogs/2012/09/04/the-3-things-you-should-know-about-hashcode/) points an interesting resource that explains this. Thanks for helping me out. – Luiggi Mendoza Mar 01 '13 at 07:48

4 Answers4

8

I would personally check for nullity and make the methods always return with no exceptions.

While runtime exceptions often aren't documented and can be thrown anywhere, I think it would be generally poor for them to be thrown by equals and hashCode. On the one hand, I can definitely see your point about being put in maps before being fully populated... but on the other hand it's hard to really know where equals will be called.

As lc says in comments, if you really want to throw an exception, it would be much better to throw an IllegalStateException to make it crystal clear that this was deliberate, than to let a NullReferenceException be thrown "by default" which makes it look like you just didn't think about the null scenario.

Jon Skeet
  • 1,421,763
  • 867
  • 9,128
  • 9,194
  • 2
    I agree, that `IllegalStateException` will be most applicable here. – Andremoniy Mar 01 '13 at 07:10
  • I would personally rarely check for null, unless I am validating an argument the branch didn't traverse but according to the method contract I am supposed to throw NPE. And yeah, this is prolly the only exception. NPE is a God-given gift to us, if only we allow the code to blow up _when_ and exactly _where_ null became a problem. No ID = NPE. I would, however, use `IllegalStateException` for other aspects, such as a matching ID but not a matching `version` (see [see this answer](https://stackoverflow.com/a/16382764/1268003)). – Martin Andersson Dec 23 '21 at 10:21
8

In general, the answer is "it depends".

  • If you should never see instances of the class with null for that field, then it is reasonable to allow an NPE to be thrown. The NPE indicates a bug; i.e. a situation where your informal invariant is broken.

  • If there are circumstances where an instance with a null could reasonably expected, then you should deal with the null case without throwing an exception.


In this particular case, you are apparently dealing with objects where the id field can be null if the object hasn't been persisted yet. This presents a tricky problem:

  • If you don't allow null for the id, then you have to be careful not to put non-persistent objects into a hash table.

  • If you do allow null for the id, then you have the problem that if you add an object to a hash table and THEN persist it, the hashcode may change leading to breakage of the hash table. So, now you need to defend against THAT ... by memoising the object's hashcode value in a transient field. And roughly the same problem occurs with equals. If equality changes when an object is persisted, then you had better not mix persisted and non-persisted keys in the same hash table.

Taking all of that into account, I'd advise to either throw that NPE, or not use the id fields in equals / hashcode.

Stephen C
  • 698,415
  • 94
  • 811
  • 1,216
  • Can only agree with all of what you said, except the first sentence lol. The answer to the _question_ isn't really "it depends"? Because, well... throwing an exception is allowed and is accepted. So the answer is "yes"? Hmm..., perhaps this is splitting hairs hahaha. Thank you Stephen for your awesome answer! I have a similar answer [here](https://stackoverflow.com/a/16382764/1268003). – Martin Andersson Dec 23 '21 at 10:09
  • 1
    Yes. You are splitting hairs. The question asked if it was "acceptable" ... not "accepted". I am not talking about acceptable by the Java compiler. (Of course it is acceptable by the compiler. That is obvious!) I am talking about acceptable by the people who are going to review the OP's code. People who might say "but `null` is an allowable value for field `x` and your `hashCode` and `equals` methods should not crap out". – Stephen C Dec 23 '21 at 10:28
2

For validating the state of the object you should use Bean validation framework to make sure the state of the object is valid.

No hashcode and equals method should not throw Exceptions.

equals method must have checks for nullity. And when the object is created it is creators responsibility to make sure that the object is in valid state, so hashCode would never have to throw an exception. For that bean validation can be used. IMO.

UPDATE: As you are using bean frameworks which create beans for you, you have to rely on bean validation. But otherwise it MUST be the responsibility of the Factory that creates the object to make sure that only a valid instance is created.

Narendra Pathai
  • 41,187
  • 18
  • 82
  • 120
1

You never want a null pointer exception in your code. Never. Especially in functions heavily-used outside your own code. hashcode equals toString should never throw exceptions.

BTW: you can always just return id as the hashcode.

Dariusz
  • 21,561
  • 9
  • 74
  • 114
  • He can't returm 'id' as thee hashCode if 'id' is a object with its own hashCode() method, which it clearly is. – user207421 Mar 01 '13 at 09:43
  • @EP the id is an Integer, as stated in the post. If the object is not null it's value can be returned and it will be a valid hashCode fulfilling all hashCode requirements. – Dariusz Mar 01 '13 at 09:51