0

In my case I have this to manipulate a shallow copy of given HashMap

public class SomeClass
{
    private HashMap<String, String> hashMap;
    public SomeClass( private HashMap<String, String> hashMap )
    {
        this.hashMap = (HashMap<String, String>) hashMap.clone();
    }
}

However eclipse suggests me to extract to local variable or method or add a cast and when I do so it keeps suggesting me same solutions :)

I've came to this post and from the accepted answer I dont find it very clear

"you're wasting memory with the new HashMap creation call"

And when I apply that to my code I'll have

if(songData.clone() instanceof HashMap)
{
    this.songData = (HashMap<String, String>) songData.clone();
}

I find that cosume more process as calling clone() twice.

Is there a better way to do this? Could my snippet be harmful to resouces?

Community
  • 1
  • 1
Ahmad Dwaik 'Warlock'
  • 5,953
  • 5
  • 34
  • 56
  • 2
    The "you're wasting memory with the new HashMap creation call" refers to the `new` in the `private Map someMap = new HashMap();` Your code does not have this problem. – Sergey Kalinichenko Dec 24 '13 at 14:06
  • 1
    Logically, you don't need to do the `instanceof` check here since `HashMap#clone()` will always return an object of type `HashMap`. However since it indirectly overrides `Object#clone()` the actual return type of the `HashMap#clone()` method is `Object` and not `HashMap` which is why you're getting a warning. I personally would not worry about the warning. – SamYonnou Dec 24 '13 at 14:12
  • Yes thanks, Is there a better way to achieve this from memory, speed and reliability wise? – Ahmad Dwaik 'Warlock' Dec 24 '13 at 14:12
  • 1
    I would probably just use `new HashMap(hashMap)` instead of using `clone()` : http://stackoverflow.com/questions/2356809/shallow-copy-of-a-map-in-java – SamYonnou Dec 24 '13 at 14:15

1 Answers1

1

Don't use clone. clone() is a badly designed method that requires too much knowledge of the object being cloned and is often implemented wrong by users anyway. (See Effective Java for more info about this)

The best way to copy the Map is to create a new map with the same data:

public SomeClass( HashMap<String, String> hashMap )
{
    this.hashMap = new HashMap<String,String>(hashMap);
}

Also as an FYI, public SomeClass( private HashMap<String, String> hashMap ) doesn't compile. You can't have parameters with a private keyword. Removing "private" from the parameter works.

you're wasting memory with the new HashMap call

This was in the answer to the other post you referenced because their code was:

private Map<String, String> someMap = new HashMap<String, String>();
someMap = (HashMap<String, String>)...

Where someMap was created with a call to new Hashmap() but then that reference was immediately replaced with a different HashMap.

It is irrelevant to your problem so you can ignore it.

dkatzel
  • 31,188
  • 3
  • 63
  • 67
  • What about the copy constructor itself? in official documentation they say to use clone for a shallow copy, and said nothing about copy constructor, and I can't try it now to test. btw "private" in argument was pasted by mistake :) – Ahmad Dwaik 'Warlock' Dec 24 '13 at 21:15
  • I also like the `putAll()` solution on second answer to this post http://stackoverflow.com/questions/2356809/shallow-copy-of-a-map-in-java but I've got no idea about its performance – Ahmad Dwaik 'Warlock' Dec 24 '13 at 21:27