Fix generics and some formatting - #59
Conversation
|
|
||
| // if we do not know the type, access Map directly | ||
| Map<String, Object> data1Map = (Map<String, Object>) data1; | ||
| Map<?, ?> data1Map = (Map<?, ?>) data1; |
There was a problem hiding this comment.
Can you explain why you've gone from a typed string/object to wildcards?
There was a problem hiding this comment.
The goal was to correct the code and fix about 50+ compiler warnings (that show up in IDEA, but only 1 "uses unchecked or unsafe operations" shows in console without -Xlint:all).
Map<?, ?> data1Map isn't untyped. A raw Map would be untyped. But, (Map<String, Object>) is unchecked. The ? removes the bad unchecked operations while keeping the compiler checks. (Map<?, ?>) is a checked cast which is strictly better than an unchecked cast, all without losing information. The @SuppressWarnings is for suppressing when we can't get rid of the unchecked cast.
I tried to follow best practices of Effective Java https://www.informit.com/articles/article.aspx?p=2861454&seqNum=2.
Eliminate every unchecked warning that you can. If you eliminate all warnings, you are assured that your code is typesafe, which is a very good thing. It means that you won’t get a
ClassCastExceptionat runtime, and it increases your confidence that your program will behave as you intended.
There was a problem hiding this comment.
Changing to I think is simply saying we have no ideas about type so there are no possible warnings to throw. This means then any .get operations aren't checked versus the original.
There was a problem hiding this comment.
Since the (Map<String, Object>) data1 is an unchecked cast it can never add a check since that's what unchecked means.
It can do nothing except possibly raise a ClassCastException. Here,get's key is always Object and V is Object, so they are both just Object.
The only feature it's adding is a theoretical exception and no check. That's why I quoted Effective Java before.
| public AbstractJiraServerInfo parse( Object object ) | ||
| { | ||
| Map<String, Object> map = (Map<String, Object>) object; | ||
| Map<?, ?> map = (Map<?, ?>) object; |
There was a problem hiding this comment.
I still think it would be better to use @SuppressWarnings( "unchecked" ) here rather than the wildcard parameterised type ; this would then match your changes in e.g. Registry.java and ParseUtils.
There was a problem hiding this comment.
I think I have a simple way to explain it. The problem is where the cast is (first line vs. second line):
Wrong way:
Map<String, Integer> map = (Map<String, Integer>) obj; // wrong: unchecked cast
Integer i = map.get( "x" ); // compiler can't verify this, can throw ClassCastExceptionRight way:
Map<?, ?> map = (Map<?, ?>) obj;
Integer i = (Integer) map.get( "x" ) // checked cast hereWhen I use @SuppressWarnings("unchecked") in Registry and ParseUtils, that was where it was safe.
In other words, cast the get not the Map.
50702e3 to
ee580c0
Compare
2 participants
No description provided.