This may be a bit of a niche case, but I wanted to suggest it anyway:
When converting an object to its String representation, the Java API offers multiple options. For primitives, we have a static toString() method overload in their corresponding wrapper class that takes the primitve value to convert:
Boolean.toString(boolean)
Character.toString(char)
Byte.toString(byte)
Short.toString(short)
Integer.toString(int)
Long.toString(long)
Float.toString(float)
Double.toString(double)
However, the String class offers several overloads of valueOf() for some of them that do effectively the same:
String.valueOf(boolean)
String.valueOf(char)
String.valueOf(int)
String.valueOf(long)
String.valueOf(float)
String.valueOf(double)
This may not seem like a problem at first, until you start refactoring your code and decide that it may be a good idea to replace your primitive data types with individual value-based wrapper classes.
(Short motivation behind such classses: Using a class like CustomerId that holds a single numeric value can make an API easier to work with instead of passing primitive values around everywhere, because you get compile-time checking and IDE support with autocompletion when passing parameters around. And if your requirement about the ID changes later, like requiring Strings instead of numeric values for the customer ID, you only need to change the variable in one place in the wrapper class instead of everywhere in your code base. And with Java’s Project Valhalla, such value-based classes will hopefully have even better performance and memory footprint.)
So instead of
public void foo(long customerId) {
[...]
strMap.put("key", String.valueOf(customerId)); //NONCOMPLIANT: Primitive overload of String.valueOf()
[...]
}
you now have something like
public void foo(CustomerId customerId) {
[...]
strMap.put("key", String.valueOf(customerId)); // -> Could result in com.example.CustomerId@6d06d69c
[...]
}
And here comes the pitfall: Because String.valueOf(Object) is one of the possible overloads, you will not get a compiler error after this refactoring. But in this case you would not get the string representation of the numeric value inside the wrapper class, but just the result of calling CustomerId.toString(), which could have any amount of additional information added to it (if it’s implemented at all). If you use this in a place where you later need to parse the String back into a number, this will silently introduce a bug.
The correct way with the refactored code would be:
public void foo(CustomerId customerId) {
[...]
strMap.put("key", Long.toString(customerId.getPrimitive())); //COMPLIANT: Wrapper.toString()
[...]
}
String.valueOf(Object) // COMPLIANT: null-safe way of calling Object.toString().
String.valueOf(char[]) // COMPLIANT: not a primitive either.
Personally, I have resolved to never use String.valueOf(primitive) in our code anymore because we have been burned by it during refactoring several times and am considering to use templated Sonar rules for each of the overloads that we want to avoid.
I do concede that it may be a bit of an edge case for other projects, though. And that it may raise quite a few issues in existing code bases that do not care about such distinctions.
Type: Code Smell
Tags: Pitfall
