views:

826

answers:

11

If I get a NullPointerException in a call like this:

someObject.getSomething().getSomethingElse().
    getAnotherThing().getYetAnotherObject().getValue();

I get a rather useless exception text like:

Exception in thread "main" java.lang.NullPointerException
at package.SomeClass.someMethod(SomeClass.java:12)

I find it rather hard to find out wich call actually returend null, often finding myself refactoring the code to something like this:

Foo ret1 = someObject.getSomething();
Bar ret2 = ret1.getSomethingElse();
Baz ret3 = ret2.getAnotherThing();
Bam ret4 = ret3.getYetAnotherOject();
int ret5 = ret4.getValue();

and then waiting for a more descriptive NullPointerException that tells me which line to look for.

Some of you might argue that concatening getters is bad style and should be avoided anyway, but my Question is: Can I find the bug without changing the code?

Hint: I'm using eclipse and I know what a debugger is, but I can't figuere out how to apply it to the problem.

My conclusion on the answers:
Some answers told me that I should not chain getters one after another, some answers showed my how to debug my code if I disobayed that advice.

I've accepted an answer that taught me excactly when to chain getters:

  • If they cannot return null, chain them as long as you like. No need for checking != null, no need to worry about NullPointerExceptions (be warned that chaining still vialotes the law of demeter, but I can live with that)
  • If they may return null, don't ever, never ever chain them, and perform a check for null values on each one that may return null

This makes any good advice on actual debugging useless.

+3  A: 

I generally do not chain getters like this where there is more than one nullable getter.

If you're running inside your ide you can just set a breakpoint and use the "evaluate expression" functionality of your ide on each element successively.

But you're going to be scratching your head the moment you get this error message from your production server logs. So best keep max one nullable item per line.

Meanwhile we can dream of groovy's safe navigation operator

krosenvold
+1  A: 

You may want to refer to this question about avoiding != null.

Basically, if null is a valid response, you have to check for it. If not, assert it (if you can). But whatever you do, try and minimize the cases where null is a valid response for this amongst other reasons.

cletus
+1  A: 

The answer depends on how you view (the contract of) your getters. If they may return null you should really check the return value each time. If the getter should not return null, the getter should contain a check and throw an exception (IllegalStateException?) instead of returning null, that you promised never to return. The stacktrace will point you to the exact getter. You could even put the unexpected state your getter found in the exception message.

Ronald Blaschke
Getters should never contain side-effect logic, this muddles the code and causes even more confusion than the original problem. Especially since a popular testing paradigm to take is to not test the getters and setters, since they're supposed to be simple.
MetroidFan2002
@rblasch: I really like that answer, as it points me to the original problem: There are methods which may return null, and others that may not. I was not always aware of this 'contract' thingy.
Brian Schimmel
@MeteroidFan2002: I agree that getters should not contain side effect logic, but what rblasch suggests does not seem like a side effect to me. A side effect would be changing the inner state of the object, or something like that. It seems a rather good idea to check if it fullfills its contract.
Brian Schimmel
Brian got this completely right. MetroidFan2002, note that I'm not proposing any side effects, which we all agree would be bad. By the way, the same rationale applies to setters, but I'd use an IllegalArgumentException there. If all accessors must be dumb I see little value over public fields.
Ronald Blaschke
Getters and setters are used in reflection, that's why they are autogenerated. Fields don't play nicely with reflection. Getters and setters should be dumb if possible. Setters could do a side-effect, but having it in your getter will cause more confusion than it solves.
MetroidFan2002
An exception is a side-effect, by the way. A getter is supposed to get the property, anything other than that and it is a side effect.
MetroidFan2002
Why don't fields "play nicely" with reflection? And no, that's not a side effect. A side effect is an action that changes something outside of a method. Changing a global variable, the state of an object or writing to a log is a side effect. An exception instead of returning a value isn't.
Ronald Blaschke
Fields don't play nicely with reflection because most reflection apis are built upon a property inspection methodology that conforms to the Javabean contract. I.e., setters and getters. And it is a side-effect because it performs an action that is not the intent of a method, ...
MetroidFan2002
A getter's purpose is to return a property. An exception that the property is null is a side-effect of invoking the getter, because it is something that occurred within the getter method invocation that forces the caller to adapt to its effects directly, instead of indirectly via a null check.
MetroidFan2002
I understand the JavaBeans argument, but please don't call it "fields don't play nicely with reflection." Regarding side effects, I'm using the term in a CS sense, which is different from API conventions, which we probably won't agree on here (and don't have to.) ;-)
Ronald Blaschke
+7  A: 

In IntelliJ IDEA you can set exceptionbreakpoints. Those breakpoints fire whenever a specified exception is thrown (you can scope this to a package or a class).

That way it should be easy to find the source of your NPE.

I would assume, that you can do something similar in netbeans or eclipse.

EDIT: Here is an explanation on how to add an exceptionbreakpoint in eclipse

Mo
You can set exception breakpoints in Eclipse as well. You cannot scope them by class/package but that should not be a problem in this case because the breakpoint needs to be activated only one execution reaches the chained getters.
Hemal Pandya
This technique is only useful if your tests identify the source of every possible NPE. In production you cannot run your code in a debugger so any NPE will still generate an unhelpful stack trace.
ewalshe
+2  A: 

Early failure is also an option.

Anywhere in your code that a null value can be returned, consider introducing a check for a null return value.

public Foo getSomething()
{
  Foo result;
  ...
  if (result == null) {
    throw new IllegalStateException("Something is missing");
  }
  return result;
}
ewalshe
Agreed -- but throw NullPointerException() if the problem is a null reference!
Neil Coffey
If the result of a computation or database lookup is null, when I expected something different then IllegalStateException is a valid choice.
ewalshe
A: 

Chained expressions like that are a pain to debug for NullPointerExceptions (and most other problems that can occur) so I would advise you to try and avoid it. You have probably heard that enough though and like a previous poster mentioned you can add break points on the actual NullPointerException to see where it occurred.

In eclipse (and most IDEs) you can also use watch expressions to evaluate code running in the debugger. You do this bu selecting the code and use the contet menu to add a new watch.

If you are in control of the method that returns null you could also consider the Null Object pattern if null is a valid value to return.

willcodejavaforfood
+1  A: 

Here's how to find the bug, using Eclipse.

First, set a breakpoint on the line:

someObject.getSomething().getSomethingElse().
getAnotherThing().getYetAnotherObject().getValue();

Run the program in debug mode, allow the debugger to switch over to its perspective when the line is hit.

Now, highlight "someObject" and press CTRL+SHIFT+I (or right click and say "inspect").

Is it null? You've found your NPE. Is it non-null? Then highlight someObject.getSomething() (including the parenthesis) and inspect it. Is it null? Etc. Continue down the chain to figure out where your NPE is occurring, without having to change your code.

MetroidFan2002
A: 

Place each getter on its own line and debug. Step over (F6) each method to find which call returns null

Sathish
+6  A: 

If you find yourself often writing:

a.getB().getC().getD().getE();

this is probably a code smell and should be avoided. You can refactor, for example, into a.getE() which calls b.getE() which calls c.getE() which calls d.getE(). (This example may not make sense for your particular use case, but it's one pattern for fixing this code smell.)

See also the Law of Demeter, which says:

  • Your method can call other methods in its class directly
  • Your method can call methods on its own fields directly (but not on the fields' fields)
  • When your method takes parameters, your method can call methods on those parameters directly.
  • When your method creates local objects, that method can call methods on the local objects.

Therefore, one should not have a chain of messages, e.g. a.getB().getC().doSomething(). Following this "law" has many more benefits apart from making NullPointerExceptions easier to debug.

Ross
+1  A: 

If you're having to get to the point where you're splitting up the line or doing elaborate debugging to spot the problem, then that's generally God's way of telling you that your code isn't checking for the null early enough.

If you have a method or constructor that takes an object parameter and the object/method in question cannot sensibly deal with that parameter being null, then just check and throw a NullPointerException there and then.

I've seen people invent "coding style" rules to try and get round this problem such as "you're not allowed more than one dot on a line". But this just encourages programming that spots the bug in the wrong place.

Neil Coffey
I don't see how any early checking can be done without breaking the single statement into multiple, essentially following the style rule you disapprove of.
Hemal Pandya
It's more about WHERE you break it up: I'm talking about null checks at the beginning of constructors and methods before the actual logic of the code starts, so that you're not trying to break lines up in the MIDDLE of a method/piece of logic.
Neil Coffey
I am afraid I am not getting it. Can you please clarify what null checks you would use in this case before the changed getters of someObject?
Hemal Pandya
+4  A: 

NPE is the most useless Exception in Java, period. It seems to be always lazily implemented and never tells exactly what caused it, even as simple as "class x.y.Z is null" would help a lot in debugging such cases.

Anyway, the only good way I've found to find the NPE thrower in these cases is the following kind of refactoring:

someObject.getSomething()
          .getSomethingElse()
          .getAnotherThing()
          .getYetAnotherObject()
          .getValue();

There you have it, now NPE points to correct line and thus correct method which threw the actual NPE. Not as elegant solution as I'd want it to be, but it works.

Esko
Wow, didn't ever try that because it seemed too easy. I just thought Java would still treat it like one single line. I will have a try on it...
Brian Schimmel