3

I am making a HashMap with keys T and values Long, and my remove method (which is overrided from the AbstractCollection class) looks like this:

 public boolean remove(Object o) {
    if(denseBag.containsKey(o)){
        if(denseBag.get(o) == 1L){
            denseBag.remove(o);
        } else {
            Long removed = denseBag.get(o);
            T theO = (T) o;
            denseBag.replace(theO, removed, removed--); 
        }
    } else {
        return false;
    }

I am getting the message: "Type safety: Unchecked cast from Object to T". I just want to make sure that it will work OK. Thank you.

ana
  • 41
  • 1
  • 3
  • What is the declared type of `denseBag`? `HashMap`? While the code probably works (depending on the context), I wouldn’t be too happy with it or would at least consider improvements, so I appreciate your question. Would it be possible to make it a `Map`? – Ole V.V. Oct 22 '16 at 20:11
  • It’s an aside, but to make sure the equality test works correct — no, to make sure the reader also is acertained that the equality test works correctly, I’d prefer either `denseBag.get(o).longValue() == 1L` or `denseBag.get(o).equals(1L)`. – Ole V.V. Oct 22 '16 at 20:15
  • Or, what would really improve the situation, would it be acceptable to change the signature of your method to `public boolean remove(T o)`? – Ole V.V. Oct 22 '16 at 20:20
  • 1
    It is a HashMap. For your second comment: denseBag.get(o).longValue == (1L), wouldn't there be an automatic unboxing of the Long? Im assuming that is why there is no compile error. Also, I am overriding the equals method so the parameter needs to stay (Obejct o) – ana Oct 22 '16 at 21:43
  • As a reader I get in doubt whether there is an automatic unboxing of `denseBag.get(o).` or an automatic boxing of `1L`. The latter may give incorrect results sometimes, so even though you haven’t seen any error yet, doesn’t mean one is not going to happen. – Ole V.V. Oct 23 '16 at 07:20
  • I should make explicit; throughout we’re assuming `T` is a type parameter. – Ole V.V. Oct 23 '16 at 07:27

2 Answers2

7

It will. Java uses Object o signature in Collections for legacy reasons.
If that bothers you still, use @SuppressWarnings("unchecked").

You'll still have a lot of other troubles, though.

This will be ignored: removed--, use --removed

Alexey Soshin
  • 16,718
  • 2
  • 31
  • 40
  • Nice catch of `removed--`, you are correct about it. Good practice says you should put `@SuppressWarnings` around so little code as possible, so you may want to factor out the cast into a separate method just for this purpose. The point is then the tag doesn’t unintentionally catch other unchecked casts, that is, you will still discover if one more creeps in. – Ole V.V. Oct 23 '16 at 07:24
  • It's not "for legacy reasons". `remove` only cares about equality, and any objects can be equal, so `Object` is the appropriate type. – newacct Oct 24 '16 at 05:40
  • 2
    Beg to differ. There is no point in trying to remove a key that is not a `T` from a `Map` since you can know in advance it is not there. In Java 1.4 and earlier, before generics, `remove(Object)` was necessary; today only code written back then still needs that. So “for legacy reasons” is to the point. – Ole V.V. Oct 24 '16 at 12:06
  • @OleV.V.: Please use `@` when replying to comments. No, there is very much a point because `remove` is not meant for removing the identical object you pass in -- it is meant for removing any keys that are *equal*. You *are* removing a key that is a `T`, but you don't have to pass in a `T` to be able to remove a `T`. – newacct Oct 25 '16 at 21:21
  • @OleV.V.: And "legacy reasons" don't make any sense because all "code written back then" will still work, as they will use raw types. All APIs where they are meant to just take a `T` are changed to `T`, like `add(T)`, and `put(K,V)`, etc. According to your logic, all those APIs must take `Object` "for legacy reasons" which is obviously not the case. All `remove` APIs in the collections framework take `Object` because that is the appropriate type for the what the methods are defined to do. – newacct Oct 25 '16 at 21:31
  • @newacct, sorry that I forgot the @ tag. We aren’t going to agree, that’s OK. Links: [Why aren't Java Collections remove methods generic?](http://stackoverflow.com/questions/104799/why-arent-java-collections-remove-methods-generic). I saw [your answer](http://stackoverflow.com/a/859239/5772882) too. – Ole V.V. Oct 26 '16 at 04:46
1

Your code will work correctly from all I can tell. If you can, I suggest you change the declaration of your method to

public boolean remove(T o)

With this you should not need the unchecked cast, which would make your code a little bit simpler. If you cannot and you believe us when we say your code is correct, use @SuppressWarnings("unchecked") as Alexey Soshin said. And consider factoring the cast out into a separate method castToT(Object obj) or what you wish to call it. The advantage of the tag is next time you get a friendly warning from your compiler or IDE, you know it’s not just an old one that you have chosen once to ignore.

Ole V.V.
  • 81,772
  • 15
  • 137
  • 161