Justified Warning?
Hi.
Consider the following code:
List list = new ArrayList();
Iterator i = list.iterator();
String s = null;
if (i.hasNext()) {
s = "Hello";
}
while (i.hasNext()) {
boolean b = s.equals("World");
}
IntelliJ warns me about s.equals("World") that it could produce a
NullPointerException.
In reality, you can see that it is not true, as if list is empty neither the
if nor the while will be executed, and if list is not empty, then both of
them will be executed.
Is that to be considered as a bug?
Thanks,
Amnon
Please sign in to leave a comment.
No, this is not a bug.
You seem to assume that the method hasNext() does not have any side-effect, so calling it successively on the same instance always produces the same result. While this is indeed the case with hasNext() in Iterator, it is not necessarily the case in general - and it would virtually impossible for IDEA (or any other static analysis tool) to determine, in the general case, whether a function has side-effects or not.
OTOH, I agree that getting the NPE warning is annoying and unuseful in your situation. I would suggest to rewrite the code like this:
if (i.hasNext()) {
s = "Hello";
do {
boolean b = s.equals("World");
...
} while (i.hasNext());
}
You're not getting any warning with this code, right?
That would require idea to have knowledge about the semantics of the Iterator class:
calling hasNext() returns the same result, unless next() wasn't called inbetween.
I think that's a bit too much to be asked from a static code analysis.
? wrote:
>That would require idea to have knowledge about ..
>I think that's a bit too much to be asked from a static code analysis.
>
.., but not from IDEA.
There are other places where some extra hardcoded smartness would be
useful, and not really far-fetched :
http://www.intellij.net/tracker/idea/viewSCR?publicId=4475
Alain
When comparing to a literal I always do it the other way round:
World won't ever be null! (Until judgement day that is ;)
Theoretically, successive calls to hasNext() on an Iterator object could return different results even if there is no next() call inbetween. (Maybe someone has a weird implementation of Iterator; or maybe another thread removes elements from the list being iterated?)
It is likely not enough (read: safe)to simply "hardcode smartness" about the Iterator interface - I'm therefore with Peter on being reasonable about what you ask from IDEA.
One more remark: even if IDEA's warning is wrong in the original example given by Amnon, it does hint to a code smell. Don't you agree that the code refactored as I showed in my previous post is better and easier to understand?
Whatever is easier to be understood for IDEA, it is easier to be understood - and maintained - by humans!
Stephen Kelvin wrote:
>I always do it the other way round: "World".equals(s);
>World won't ever be null! (Until judgement day that is ;)
>
I even it more readable :
compare :
if ( reply.equals("yes") ) ) return 1 ;
if ( reply.equals("no" ) ) return 2 ;
if ( reply.equals("IhaveNoFxxingIdea" ) ) return 3 ;
to
if ( "yes" .equals(reply) ) return 1 ;
if ( "no" .equals(reply) ) return 2 ;
if ( "IhaveNoFxxingIdea" .equals(reply) ) return 3 ;
The important (different) information comes first.
Alain
And for the lazy, the InspectionGadgets plugin includes an inspection to find and (as of version 0.0.2) automatically fix any calls to equal with a string literal arg to have the string literal as the target.
How would you write the code differently? I mean the hasNext() part and not
the equals call (which was just a simplification of code that I have)?
I know I can do something like:
boolean initS = true;
String s;
while (i.hasNext()) {
if (initS) {
initS = false;
s = "Hello";
}
boolean b = "Hello".equals(s);
}
This would work, but it is just a shame to check the if all the time.
Remember, this is a simplification of real code. The real code has
instantiation of a class instead of s and adding of the class to another
class only if there are items in the list, so I can't just have s set to
"Hello", I need to instantiate s only if there are items in the list.
I know this is kind of a minor issue but it still seems to be a warning
about something that in real life will work.
Thanks,
Amnon
"vlad" <no_mail@jetbrains.com> wrote in message
news:29083092.1059481402761.JavaMail.itn@is.intellij.net...
could return different results even if there is no next() call inbetween.
(Maybe someone has a weird implementation of Iterator; or maybe another
thread removes elements from the list being iterated?)
>
the Iterator interface - I'm therefore with Peter on being reasonable about
what you ask from IDEA.
>
given by Amnon, it does hint to a code smell. Don't you agree that the code
refactored as I showed in my previous post is better and easier to
understand?
>
understood - and maintained - by humans!
>
Amnon I. Govrin wrote:
How about this?
List list = new ArrayList();
Iterator i = list.iterator();
String s = null;
if (i.hasNext()) {
s = "Hello";
while (i.hasNext()) {
boolean b = s.equals("World");
}
}
Have you looked over my suggested code improvement, posted before? If so, why is it not suitable in your case? I would expect this kind of refactoring to work no matter what your real code is.
Sure. My suggested improvement solves this problem, doesn't it?
Vlad
I'm also suggesting to replace the while() {...} with a do { } while() in order to save one useless test.
Hello,
In this case this list is not local variable, but the may be situation, then
"if" result was "false" and "while" is "true". Becouse this list may changed
in other thread...
If i understand by your code, you want to implement "contains" method in
your realization of ArrayList... Maybe i wrong:
Good luck!
--
Alexey Efimov, Software Engineer
Sputnik Labs,
http://www.spklabs.com
"Amnon I. Govrin" <agovrin@freshwater.com> wrote in message
news:bg42j3$ir5$1@is.intellij.net...
>
>
>
>
the
>
>
>
>
>
I eventually put the while inside the if, I guess it is at least more
readable that way.
Thanks,
Amnon
"Alexey Efimov" <aefimov@spklabs.com> wrote in message
news:bg62f2$9i7$1@is.intellij.net...
>
then
changed
>
>
>
>
of
>
>
Yes, it works in this particular case but what if I had (slightly changed
example):
List list = new ArrayList();
Iterator i = list.iterator();
String s = null;
if (i.hasNext()) {
s = fetchSomething();
}
doSomething();
while (i.hasNext()) {
boolean b = s.equals("World");
}
(Note that it might be important to call "fetchSomething()" BEFORE
"doSomething()".)
Any thoughts how to "improve" code?
--
Valentin Kipiatkov
JetBrains, Inc
http://www.intellij.com
"Develop with pleasure!"
"Amnon I. Govrin" <agovrin@freshwater.com> wrote in message
news:bg6aee$f81$1@is.intellij.net...
>
>
neither
>
>
Nice homework, Valentin :)
Here is my entry:
boolean condition = i.hasNext();
if (condition) {
s = fetchSomething();
}
doSomething();
while (condition) {
boolean b = s.equals("World");
i.next();
condition = i.hasNext();
}
The important thing to note is that IDEA is smart enough to realize that the value of condition cannot be changed by doSomething(), since it is a local variable - so IDEA does not give any NPE warning in this case!
Next question would be: is the new code really an improvement - or is it just a workaround to avoid the NPE warning? I would still maintain that it is an improvement (for once thing, it avoids an unnecessary call to i.hasNext()) - but I may be subjective here. What do you think?