7

Instead of the usual if (myString == null || myString.equals("")) I tend to prefer using the org.apache.commons.lang.StringUtils class and do if (StringUtils.isEmpty(myString)).

However this - at least the way I'm doing it - comes with a gigantic downside: because FindBugs - or the compiler warning mechanism, f. ex. from Eclipse - will no longer see the explicit null-check, it will no longer consider myString as potentially null, so it will no longer raise the warnings about potential (or sure) null pointers on it, and those warnings are extremely useful in my view.

Example (ADDED):

import org.apache.commons.lang.StringUtils;

public class TestWarning
{
    void testWarning(String myString)
    {
        //if (myString == null || myString.equals("")) // With this, the last line shows the warning.
        if (StringUtils.isEmpty(myString)) // With this, no warning.
        {
            // Anything.
        }
        int x = myString.length(); // Warning is here: "Potential null pointer access: The variable myString may be null at this location"
    }
}

So I'm just trying to make sure that there is no way to eliminate or minimize this downside so that one can use StringUtils.isEmpty and still get the null pointer warnings. Maybe some annotation like @Nullcheck to add to the isEmpty method or something else ?

ADDED: For ex., would it be feasible to create a custom annotation like @Nullcheck, add it to the arg of isEmpty like public static boolean isEmpty(@Nullcheck String str) just to indicate that the method does a null-check on that arg, and make so that the compiler warning mechanism or FindBugs treat a if (StringUtils.isEmpty(myString)) just like an explicit null-check ?

SantiBailors
  • 1,596
  • 3
  • 21
  • 44
  • 3
    StringUtils.isEmpty() handles null so why should FindBugs raise a "possible null" warning? I am not getting your issue here... – WarrenFaith Jun 05 '14 at 09:41
  • 2
    What warning of null pointers are you worried about? Can you explain that a bit more? Do you want a warning that it might throw a NPE when trying to check something like this: `StringUtils.isEmpty(null)`? Because if that's the case, then you needn't worry about it as `StringUtils.isEmpty()` handles `null` by itself and thus *FindBugs* will not bug you with unnecessary warnings. [Read the docs](http://commons.apache.org/proper/commons-lang/apidocs/org/apache/commons/lang3/StringUtils.html#isEmpty(java.lang.CharSequence)) for more info. – Rahul Jun 05 '14 at 09:41
  • 1
    I'm not worried about a null pointer inside `isEmpty`; it's just that if I do `if (myString == null)` and some lines below - outside that `if` - I do for ex. `myString.length()` I will get a compiler warning of a potential null pointer on `myString.length()`. I would not get that warning if instead of `if (myString == null)` I do `if (StringUtils.isEmpty(myString))` – SantiBailors Jun 05 '14 at 09:55
  • 1
    A compiler and even tools like FindBugs have limits and you just found one. Basically that is why you as a programmer need to use the brain you have :) – WarrenFaith Jun 05 '14 at 10:10
  • Right. But that would apply to a lot of constructs and tools designed to minimize mistakes - accidental or due to ignorance - and I kind of like that ;) – SantiBailors Jun 05 '14 at 10:15
  • Please post a working example and mark the line where you expect a warning. It sounds like you found a bug if the usage of the variable is outside the `if` block. Better to post the actual code than describe it with prose. – David Harkness Jun 06 '14 at 01:58
  • I don't think it's a bug, just that with `if (myString == null)` the compiler or FindBugs understand that `myString` can be null, while with `if (StringUtils.isEmpty(myString))` they don't understand that, which I think is fair enough. However I added the example to my question. – SantiBailors Jun 06 '14 at 07:28
  • Why don't you simply annotate the myString argument with JSR-305 `@Nullable` (https://code.google.com/p/jsr-305/source/browse/trunk/ri/src/main/java/javax/annotation/Nullable.java?r=24) or FindBugs annotation `@CheckForNull`? – JB Nizet Jun 06 '14 at 07:32
  • @JB Nizet : Because I didn't know about either of them :) Thanks a lot, I'm going to try that and I will post the outcome. If it works I think yours should be the accepted answer. – SantiBailors Jun 06 '14 at 07:55
  • I [downloaded](http://www.java2s.com/Code/Jar/j/Downloadjsr305jar.htm) JSR-305 and tried its `@Nullable`, to no avail. I used it as per [one of their examples](https://groups.google.com/d/msg/jsr-305/mU-HqM4jwtk/Hoq1uPrykHMJ). `import javax.annotation.Nullable;` `public class TestWarning {` ` void testWarning(String myString) {` ` if (isEmpty(myString)) { /* Anything. */ }` ` int x = myString.length(); // Shows no nullness warning.` ` }` ` public static boolean isEmpty(@Nullable String str) {` ` return str == null || str.length() == 0;` ` }` `}` – SantiBailors Jun 06 '14 at 11:11
  • And sorry for the messy code above but I can't find ways to paste a readable code snippet in a comment. I read the help and tried several ways then the 5 minutes expired. I'm not sure how one is supposed to post code except using "Answer Your Question" which is of no use. – SantiBailors Jun 06 '14 at 11:20

3 Answers3

1

Whether you get a potential null pointer warning really depends on how good the static code analyzer is. Logically, if you use

if (myString == null || myString.equals(""))

your code expect myString to be null. In the subsequent line, however, you dereference myString. The static code analyzer sees these two facts, and create a null pointer warning.

If you use

if (StringUtils.isEmpty(myString))

your code does not say whether it expects myString to be null. The static code analyzer, thus, does not generate a null pointer warning. The static code analyzer can certainly choose to generate a null pointer warning, but this will produce a lot of false positives, because it needs to assume that inputs to any method may be null.

fajarkoe
  • 1,543
  • 10
  • 12
  • Agreed, that is the point. So I'm wondering whether it's possible to annotate `isEmpty` signature to let the compiler know that `if (StringUtils.isEmpty(myString))` is as much of a null-check on `myString` as `if (myString == null)` is. I tried the JSR-305 `@Nullable` suggestion above but that `@Nullable` does not seem to do that. – SantiBailors Jun 10 '14 at 07:43
1

If you annotate myString with @Nullable, i.e.

void testWarning(@Nullable String myString)

then at least eclipse will report a warning when you dereference it.

I agree that FindBugs should also be warning about it, but I can't get it to do so either.

TimK
  • 4,635
  • 2
  • 27
  • 27
  • That's what I tried with the example in the 10th comment of my question; it didn't work in Eclipse but maybe that's not the code you mean. The code I posted there didn't really come out very readable, so I'm trying again here. It should be possible to paste it into an IDE and have it format the code. `import javax.annotation.Nullable; public class TestWarning { void testWarning(String myString) { if (isEmpty(myString)) { /* Anything. */ } int x = myString.length(); /* No nullness warning */ } public static boolean isEmpty(@Nullable String str) { return str == null || str.length() == 0; }}` – SantiBailors Jun 17 '14 at 07:53
  • If you mean that I should put the `@Nullable` in the calling code (`testWarning` method in my example), then that's not what I'm talking about. If I do so, it would be me who says that `myString` could be null. I want the compiler to infer from `if (isEmpty(myString))` that `myString` could be null, just like it would infer that from an `if (myString == null)`. I want that doing `if (isEmpty(myString))` and doing `if (myString == null)` have the same effect on the warning (that is, show the warning). – SantiBailors Jun 17 '14 at 08:03
  • 1
    Yes, I meant to put `@Nullable` on the argument to testWarning. When you say `if (myString == null)` you are telling eclipse/findbugs that you know that myString might be null. If that test isn't there, another way to provide the same information is to mark myString as @Nullable. When you call `isEmpty`, even if its argument is marked @Nullable, you're not telling it that. It accepts non-null parameters, but that doesn't mean you think myString might be null. – TimK Jun 17 '14 at 22:52
  • Understood, thanks. Using `@Nullable` in that way would be a reasonable compromise; not as good as having the compiler figuring out by itself that `myString` can be null as it does with `if (myString == null)` but that's probably the closest one can get. – SantiBailors Jun 18 '14 at 07:38
1

The Eclipse code analysis can also evaluate external null annotations (since 4.5.0). More information is available at https://wiki.eclipse.org/JDT_Core/Null_Analysis/External_Annotations.

Konrad Windszus
  • 241
  • 3
  • 7