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


0
16 comments
Avatar
Permanently deleted user

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.

0
Avatar
Permanently deleted user

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?

0
Avatar
Permanently deleted user

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.

0
Avatar
Permanently deleted user

? 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


0

When comparing to a literal I always do it the other way round:

World won't ever be null! (Until judgement day that is ;)

0
Avatar
Permanently deleted user

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!

0
Avatar
Permanently deleted user

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






0
Avatar
Permanently deleted user


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.

0
Avatar
Permanently deleted user

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...

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!
>


0
Avatar
Permanently deleted user

Amnon I. Govrin wrote:

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.


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");
}
}

0
Avatar
Permanently deleted user

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)?


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.

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.


Sure. My suggested improvement solves this problem, doesn't it?

Vlad

0
Avatar
Permanently deleted user

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");
}
}


I'm also suggesting to replace the while() {...} with a do { } while() in order to save one useless test.

0
Avatar
Permanently deleted user

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:

 0) {
        // Get default iterator
        Iterator it = iterator();
        String findValue = (String)elem;

        while (it.hasNext()) {
          String value = (String)it.next();
          if (value.equals(findValue)) {
            return true;
          }
        }
      }
    }

    // If not finded return default contains results
    return super.contains(elem);
  }
}
]]>


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...

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

>
>


0
Avatar
Permanently deleted user

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...

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:

>

 import java.util.ArrayList;
> import java.util.Iterator;
>
> public class MyList extends ArrayList {
>   public boolean contains(Object elem) {
>     assert elem != null;
>     assert elem instanceof String;
>
>     synchronized (this) {
>       if (size() > 0) {
>         // Get default iterator
>         Iterator it = iterator();
>         String findValue = (String)elem;
>
>         while (it.hasNext()) {
>           String value = (String)it.next();
>           if (value.equals(findValue)) {
>             return true;
>           }
>         }
>       }
>     }
>
>     // If not finded return default contains results
>     return super.contains(elem);
>   }
> }
> ]]>

>

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...

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

>
>

>
>


0
Avatar
Permanently deleted user

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...

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...

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:

>

 > import java.util.ArrayList;
> > import java.util.Iterator;
> >
> > public class MyList extends ArrayList {
> >   public boolean contains(Object elem) {
> >     assert elem != null;
> >     assert elem instanceof String;
> >
> >     synchronized (this) {
> >       if (size() > 0) {
> >         // Get default iterator
> >         Iterator it = iterator();
> >         String findValue = (String)elem;
> >
> >         while (it.hasNext()) {
> >           String value = (String)it.next();
> >           if (value.equals(findValue)) {
> >             return true;
> >           }
> >         }
> >       }
> >     }
> >
> >     // If not finded return default contains results
> >     return super.contains(elem);
> >   }
> > }
> > ]]>

>

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...

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

>
>

>
>

>
>


0
Avatar
Permanently deleted user

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?

0

Please sign in to leave a comment.