Rule for missing null check on Java Map.Get()

Rules and Languages New rules / language support
1

Description of the Rule

Some key-value map implementations return null if a key is non-existent (eg, HashMap), while other can throw some key-does-not-exist exception.
In both cases, retrieving and using the returned value without any prior checks is a code smell and can lead to unexpected null pointer exceptions (or non-existent key exceptions).

It is a good practice to check for null before using the returned value, or check the existence of the key before retrieving it.

“what could be its impact?” and “what could happen in case of a successful attack?” (for Vulnerabilities and Security Hotspots)

N/A

Snippet of Noncompliant Code

String toUpperCase(Map<String, String> map, String key) {
    return map.get(key).toUpperCase(); // Noncompliant
}

Snippet of Compilant Code

String toUpperCase(Map<String, String> map, String key) {
    var res = return map.get(key);
    if (res != null) {
        return return res.toUpperCase();
    }
    return "";
}

or

String toUpperCase(Map<String, String> map, String key) {
    if (map.containsKey(key)) {
        return map.get(key).toUpperCase();
    }
    return "";
}

or

String toUpperCase(Map<String, String> map, String key) {
    return map.getOrDefault(key, "").toUpperCase(); // we could also use Map.computeIfAbsent()
}

Exceptions to the Noncompliant Code

None that I can think of.

External references and/or language specifications

Type

Code Smell

Tags

Java, NullPointerException

1 Like
2

I see some potential for false-positives (or at least annoyances) in such a rule.

What if you were to just outright return the value from the Map, and the calling code in some other function may be responsible for doing the null check?

Or when you are certain that your Map always contains a mapping for a specific key and that it would be unnecessary boilerplate to explicitly check for its presence?
And where you would actually want to have an exception instead of some fallback that hides away the actual problem, but where getting a NullPointerException is good enough to point you at the problem and adding your own dedicated exception handling to every use of a Map would be overkill.

1 Like
3

“What if you were to just outright return the value from the Map”

In that case surely the rule would not be triggered as there is no usage of the returned value.

And if the other behaviours are what you desire then surely the rule can be deactivated from your ruleset.

I think that in most cases blindly using a value from a Map is not good practise, in which case such a rule would be useful.

Kind regards,

6

Hi @jantunes,

Welcome to the community,

Thanks for starting this discussion and sharing your rule idea. You’ve correctly identified that using the result of map.get() without a check is a common source of NullPointerExceptions, and your examples illustrate this well.

The follow-up points raised by @CrushaKRool about potential false positives are also legitimate. This highlights the core challenge: inferring the programmer’s intention with Java’s standard Map.get(). Sometimes the key is expected to be present and if it is not present it is a bug. In this case, I think failing fast is the most appropriate action (an NPE is acceptable). Other times, the key might be present or not and performing a check is necessary.

In Java, it is difficult to guess the intention of the programmer. In other languages, it is different. For instance, in Scala map(key) raises an exception if the key does not exist and map.get(key) returns null.

Introducing the rule risks creating a lot of noise and we prioritize keeping false positives low. We’ll hold off on adding this specific rule for now. However, we will keep track of the idea and if at some point a Map.getExist method [or similar] is added to the standard library, we will implement the rule.

Thank you for reaching out with this rule idea and to provide this level of details.

1 Like