difference between changing the String after putting in Set or changing Set in Object

[Amandeep Singh]

example-

if you see its output, you will not see str changes.

Set<String> set = new HashSet<String>();
String str = "hello";
set.add(str);
str = "world";
System.out.println(set);

but if you do this way, you can see the changes in set values but not in String value. well one reason that comes into my mind is String is immutable whereas Set is mutable. So in my production changes also i followed the below steps.

Is it a kind of risky thing to do or pretty safe to do this ?

Set<String> set = new HashSet<String>();
	String str = "hello";
	ContData contData = new ContData(set);
	set.add(str);
	System.out.println(contData.getSet());

import java.util.Set;


public class ContData {
Set<String> set;

public ContData(Set<String> set) {
	this.set = set;
}

public Set<String> getSet() {
	return set;
}

public void setSet(Set<String> set) {
	this.set = set;
}
}

[Duc Vo]

Amandeep Singh wrote:example-
Is it a kind of risky thing to do or pretty safe to do this ?

It is risky, but it is acceptable in some cases. It is risky because the ContData doesn't have full control of its internal state, hence won't be able to ensure that its operations are consistent. So in the perfect world, such problem should not exist, however, sometime it is too expensive to implement otherwise, you just have to deal with it, just need to be aware of the problem.

Anyway, after some refactoring your ContData can be something like

import java.util.Collection;
import java.util.HashSet;
import java.util.Iterator;
import java.util.Set;

public class ContData {
	Set<String> set;

	public ContData() {
		this.set = new HashSet<String>();
	}

	public ContData(Set<String> set) {
		this();
		if (set != null) {
			this.set.addAll(set);
		}
	}

	public void add(String item) {
		this.set.add(item);
	}
	
	public void addAll(Collection<String> col) {
		this.set.addAll(col);
	}
	
	public Iterator<String> iterator() {
		return this.set.iterator();
	}

	//test method to see its internal state
	@Override
	public String toString() {
		return this.set.toString();
	}
}

So that if you want to initialise an instance of ContData, it should be something like

    Set<String> set = new HashSet<String>();  
    String str = "hello";  
    set.add(str);  

    ContData contData = new ContData(set);  

    System.out.println(contData.toString());  

    set.add("world");
    
    //should return the same result as previous println
    System.out.println(contData.toString());  

[Amandeep Singh]

well if you look this code-

Set<String> set = new HashSet<String>();
	String str = "hello";
	ContData contData = new ContData(set);
	set.add(str);
	System.out.println(contData.getSet());

it is no different than your's except ContData.

I think as long as you see the results good in output. the possibility of riskiness should be ruled out. And my this code is in Production, it is working without the issues.

[Duc Vo]

It will only be a problem if you have to debug mysterious issue later on i.e.
you change the set and the internal state of contData change without its notice.

Set<String> set = new HashSet<String>();
	String str = "hello";
	ContData contData = new ContData(set);
	set.add(str);
	System.out.println(contData.getSet());

        //if you change the set like few screens below the above code
        set.add("world");

        //then this line isn't what you are expecting anymore.
	System.out.println(contData.getSet());

Ah by the way, this

So that if you want to initialise an instance of ContData, it should be something like

Should read

If you change the set outside contData, it doesn't change the internal state of contData

[Amandeep Singh]

Duc Vo wrote:

So that if you want to initialise an instance of ContData, it should be something like

Should read

If you change the set outside contData, it doesn't change the internal state of contData

i don't agree with this point. if we change the internal state it is definitely reflected. could you somehow can give me a scenario where it doesn't works..

[Duc Vo]

Amandeep Singh wrote:
i don't agree with this point. if we change the internal state it is definitely reflected. could you somehow can give me a scenario where it doesn't works..

Didn't you try my example above?

With your version of ContData, if I add another item to the set after initialising contData object, then the item is added into internal set of contData without its notice. And it doesn't happen to my version of ContData.

In the above example, your output at line 5 will be [hello] and after changing the set at line 8, the output at line 11 is now [hello, world]. Which means the internal state of object contData has been changed without using its own operation, it actually violates the Encapsulation principle of OOD.

In my example in the second post, the output at line 7 is [hello] and even if I changed the set at line 9 the output at line 12 is still [hello].

[Amandeep Singh]

In the above example, your output at line 5 will be [hello] and after changing the set at line 8, the output at line 11 is now [hello, world]. Which means the internal state of object contData has been changed without using its own operation, it actually violates the Encapsulation principle of OOD.

this is what i actually want. I am asking is this thing guaranteed to work always ? Let me know as it is an urgent issue. what is working in above quote is what i need always to work.

[Duc Vo]

I think if you only run them in the same JVM, it should always work.

But it's not a good practice, and potentially will create more problems later on say a year from now and/or when other developers inherit the project.
Should you let the contData object handle the change instead?

[Amandeep Singh]

even if ContData want to handle the situation. it will be eventually end up in same situation.

the only way issue resolve is-

package rough;

import java.util.HashSet;
import java.util.Set;

public class MainContData {

    public static void main(String[] args) {
        Set<String> set = new HashSet<String>();
        String str = "hello";
        ContData contData = new ContData(set);
        set.add(str);
        System.out.println(contData.getSet());
        //if you change the set like few screens below the above code
        set.add("world");

        //then this line isn't what you are expecting anymore.
        System.out.println(contData.getSet());

    }
}

can be changed to

package rough;

import java.util.HashSet;
import java.util.Set;

public class MainContData {

    public static void main(String[] args) {
        Set<String> set = new HashSet<String>();
        String str = "hello";
        set.add(str);
        set.add("world");
        ContData contData = new ContData(set);
        System.out.println(contData.getSet());
    }
}

do you think the above code changes does it really makes difference?

[Duc Vo]

No that change doesn't make any difference. The problem isn't with the client code, the problem is with the implementation of ContData. If later on in the code the set variable is used for different purpose, the contData object will not have control of its state. The ContData should have method to add an item or collection of items, it should also provide a way to iterate through the items if the client code is required, also it should not return a reference to the set (its internal state). Recheck my implementation of the ContData in the second post.

[Amandeep Singh]

oh i understand your point, you want to make the Set localized to ContData. So nobody can change it without the intent of ContData.
but the way i did is just made the Set outside the ContData. So that it can be shared outside also.

do you think there can be issues in my code, if yes which scenario ?

I just treated ContData as a placeholder of set.

[Duc Vo]

Amandeep,

I did mention that there may be side effect problem and I did give example in previous posts.
Normally it should work, but if you have mysterious issue like getSet() method of ContData returns inconsistent data time to time, be aware that any one who got a hold on the set reference of ContData can change the set. If it only happen on few places in your project, then it should be managable. If it happens almost to all your objects, you should consider refactor your code base.

Hope it help,

[Amandeep Singh]

well, i change the set only at one place as same as in my examples. but to get the set value, i do it at various places.
Does any one agree to this, for this particular scenario i mentioned it can give rise to problems. So far my tests are working great.
does that mean still i haven't discovered the issue.

Experts Help is appreciated!

[Rob Spoor]

Duc Vo wrote:Normally it should work, but if you have mysterious issue like getSet() method of ContData returns inconsistent data time to time, be aware that any one who got a hold on the set reference of ContData can change the set.

That's where the unmodifiable wrapper methods in java.util.Collections come in:

public Set<String> getSet()
{
    return Collections.unmodifiableSet(set);
}

public Iterator<String> iterator()
{
    return getSet().iterator();
}

Note how my iterator() method uses the unmodifiable set returned by getSet(). That's because Iterator has a nasty method called remove() - which allows foreign code to remove elements from your set if you return set.iterator() directly!

[Amandeep Singh]

In my case, there is no method which is going to remove from the Set.
but with the situation i am fighting is -


but the way i did is just made the Set outside the ContData. So that it can be shared outside also. So the code outside the ContData can change the value of Set.

but the way Duc is saying- make Set localized to ContData. So nobody can change it without the intent of ContData.

Even i agree with Duc approach. but is my approach wrong. I tested my code, it works well. if you want complete posting of the code let me know. above code are just snapshots. As my code will go into production, please advise if it needs immediate intention.



Expert advice would be appreciated as per my problem...

[Rob Spoor]

Your code is only as flawed as the code that uses it. If in your application, there is no code at all that modifies the set - no problem. But if anybody (perhaps you in the future) decides to use that class in another project, and doesn't know the contents are shared (or willingly (ab)uses this fact), your code may not work as that person expected.

[Amandeep Singh]

thanks Rob. can you suggest some changes in my code ASAP.

[Amandeep Singh]

Well in real world i do have, more than 1 set types like this-

import java.util.Set;

public class ContData1 {
private Set<String> setContPackType;
private Set<Number> setGrossWgt;
private Set<String> setGrossWgtUom;
private Set<Number> setVol;
private Set<Number> setContPackQty;
private Set<String> setContPackUom;
/**
 * 
 */
public ContData1() {
}
/**
 * @param setContPackType
 * @param setGrossWgt
 * @param setGrossWgtUom
 * @param setVol
 * @param setContPackQty
 * @param setContPackUom
 */
public ContData1(Set<String> setContPackType, Set<Number> setGrossWgt, Set<String> setGrossWgtUom,
	Set<Number> setVol, Set<Number> setContPackQty, Set<String> setContPackUom) {
    this.setContPackType = setContPackType;
    this.setGrossWgt = setGrossWgt;
    this.setGrossWgtUom = setGrossWgtUom;
    this.setVol = setVol;
    this.setContPackQty = setContPackQty;
    this.setContPackUom = setContPackUom;
}
/**
 * @return the setContPackType
 */
public Set<String> getSetContPackType() {
    return setContPackType;
}
/**
 * @param setContPackType the setContPackType to set
 */
public void setSetContPackType(Set<String> setContPackType) {
    this.setContPackType = setContPackType;
}
/**
 * @return the setGrossWgt
 */
public Set<Number> getSetGrossWgt() {
    return setGrossWgt;
}
/**
 * @param setGrossWgt the setGrossWgt to set
 */
public void setSetGrossWgt(Set<Number> setGrossWgt) {
    this.setGrossWgt = setGrossWgt;
}
/**
 * @return the setGrossWgtUom
 */
public Set<String> getSetGrossWgtUom() {
    return setGrossWgtUom;
}
/**
 * @param setGrossWgtUom the setGrossWgtUom to set
 */
public void setSetGrossWgtUom(Set<String> setGrossWgtUom) {
    this.setGrossWgtUom = setGrossWgtUom;
}
/**
 * @return the setContPackQty
 */
public Set<Number> getSetContPackQty() {
    return setContPackQty;
}
/**
 * @param setContPackQty the setContPackQty to set
 */
public void setSetContPackQty(Set<Number> setContPackQty) {
    this.setContPackQty = setContPackQty;
}
/**
 * @return the setContPackUom
 */
public Set<String> getSetContPackUom() {
    return setContPackUom;
}
/**
 * @param setContPackUom the setContPackUom to set
 */
public void setSetContPackUom(Set<String> setContPackUom) {
    this.setContPackUom = setContPackUom;
}
/**
 * @return the setVol
 */
public Set<Number> getSetVol() {
    return setVol;
}
/**
 * @param setVol the setVol to set
 */
public void setSetVol(Set<Number> setVol) {
    this.setVol = setVol;
}
public void addSetVol(Number num){
	this.setVol.add(num);
}
public void addSetGrossWgt(Number num){
	this.setGrossWgt.add(num);
}   
public void addSetContPackQty(Number num){
	this.setContPackQty.add(num);
}   
public void addSetContPackUom(String str){
	this.setContPackUom.add(str);
}   
public void addSetGrossWgtUom(String str){
	this.setGrossWgtUom.add(str);
}   
public void addSetContPackType(String str){
	this.setContPackType.add(str);
}   

}

and the code use to above code is-

 Set<String> setContPackType = new HashSet<String>();
	    Set<Number> setGrossWgt = new HashSet<Number>();
	    Set<String>   setGrossWgtUom = new HashSet<String>();
	    Set<Number> setVol = new HashSet<Number>();
	    Set<Number> setContPackQty = new HashSet<Number>();
	    Set<String> setContPackUom = new HashSet<String>();  
        setContPackType.add("hello1");
	    ContData1 contData = new ContData1(setContPackType, setGrossWgt, setGrossWgtUom, setVol, setContPackQty, setContPackUom);
	    contData.addSetContPackType("hello");
	    setContPackType.add("world");

but i am unable to achieve the desired results. Still without the intent of ContData1, the user can changes the value of set.

[Rob Spoor]

If you don't want the sets edited outside your class, you will have to follow the combined techniques Duc Vo and I have shown you: copy the set in the constructor, and return an unmodifiable set wrapper:

public ContData1(Set<String> setContPackType, ...)
{   
    this.setContPackType = new HashSet<String>(setContPackType); // or TreeSet, or LinkedHashSet
    ....
}

/**  
 * @return an unmodifiable wrapper around the setContPackType  
 */   
public Set<String> getSetContPackType()
{   
    return Collections.unmodifiableSet(setContPackType); // so users can't add to it.
}

public void setSetContPackType(Set<String> setContPackType)
{   
    // you could also reinitialize this.setContPackType, but clearing and adding all will reuse your existing object
    this.setContPackType.clear();
    this.setContPackType.addAll(setContPackType);   
}   

Note that I also changed the Javadoc of the get method to inform users that they can't add anything to the returned set.

Set<String> setContPackType = new HashSet<String>();   
...
setContPackType.add("hello1");   
ContData1 contData = new ContData1(setContPackType, ...);   
contData.addSetContPackType("hello");    // adds to the internal set, but not the external set
setContPackType.add("world");   // adds to the external set, but not the internal set
contData.getSetContPackType().add("goodbye");  // throws UnsupportedOperationException

[Duc Vo]

Rob has said it all. Just one minor point, if you create those empty HashSet just to create Cont1Data object, you may want to let Cont1Data to create them as well. So the clients of Cont1Data don't need to know how Cont1Data store the data.

[Amandeep Singh]

So my final code is -

import java.util.HashSet;
import java.util.Set;


public class MainContData {
public static void main(String[] args) {
	    Set<String> setContPackType = new HashSet<String>();
	    Set<Number> setGrossWgt = new HashSet<Number>();
	    Set<String>   setGrossWgtUom = new HashSet<String>();
	    Set<Number> setVol = new HashSet<Number>();
	    Set<Number> setContPackQty = new HashSet<Number>();
	    Set<String> setContPackUom = new HashSet<String>();  
        setContPackType.add("hello1");
	    ContData1 contData1 = new ContData1();//now the user just creates only 1 object
	    contData1.setSetContPackType(setContPackType);//add to the internal set
	    setContPackType.add("world");//add to the external set
	    System.out.println(contData1.getSetContPackType());
	    Set<String> setContPackType1 = new HashSet<String>();//i have requirement to add new elements to the existing set
	    setContPackType1.add("new world");
	    contData1.setSetContPackType(setContPackType1);
	    System.out.println(contData1.getSetContPackType());//so here i add new elements to the existing set
}
}

import java.util.Collections;
import java.util.HashSet;
import java.util.Set;

public class ContData1 {
private Set<String> setContPackType;
private Set<Number> setGrossWgt;
private Set<String> setGrossWgtUom;
private Set<Number> setVol;
private Set<Number> setContPackQty;
private Set<String> setContPackUom;
/**
 * 
 */
public ContData1() {
	    this.setContPackType = new HashSet<String>();
	    this.setGrossWgt = new HashSet<Number>();
	    this.setGrossWgtUom = new HashSet<String>();
	    this.setVol = new HashSet<Number>();
	    this.setContPackQty = new HashSet<Number>();
	    this.setContPackUom = new HashSet<String>();  
}
//Commented out this code, now the user have to create just 1 ContData1 object, it will take care.
//public ContData1(Set<String> setContPackType, Set<Number> setGrossWgt, Set<String> setGrossWgtUom,
//		Set<Number> setVol, Set<Number> setContPackQty, Set<String> setContPackUom) {
//	    this.setContPackType = setContPackType;
//	    this.setGrossWgt = setGrossWgt;
//	    this.setGrossWgtUom = setGrossWgtUom;
//	    this.setVol = setVol;
//	    this.setContPackQty = setContPackQty;
//	    this.setContPackUom = setContPackUom;
//	}
/**
 * @return the setContPackType
 */
public Set<String> getSetContPackType() {
    return Collections.unmodifiableSet(setContPackType);
}
/**
 * @param setContPackType the setContPackType to set
 */
public void setSetContPackType(Set<String> setContPackType) {
    this.setContPackType.addAll(setContPackType);// don't want to clear it, in further code, i may need to add elements
}
/**
 * @return the setGrossWgt
 */
public Set<Number> getSetGrossWgt() {
    return Collections.unmodifiableSet(setGrossWgt);
}
/**
 * @param setGrossWgt the setGrossWgt to set
 */
public void setSetGrossWgt(Set<Number> setGrossWgt) {
    this.setGrossWgt.addAll(setGrossWgt);
}
/**
 * @return the setGrossWgtUom
 */
public Set<String> getSetGrossWgtUom() {
    return Collections.unmodifiableSet(setGrossWgtUom);
}
/**
 * @param setGrossWgtUom the setGrossWgtUom to set
 */
public void setSetGrossWgtUom(Set<String> setGrossWgtUom) {
    this.setGrossWgtUom.addAll(setGrossWgtUom);
}
/**
 * @return the setContPackQty
 */
public Set<Number> getSetContPackQty() {
    return Collections.unmodifiableSet(setContPackQty);
}
/**
 * @param setContPackQty the setContPackQty to set
 */
public void setSetContPackQty(Set<Number> setContPackQty) {
    this.setContPackQty.addAll(setContPackQty);
}
/**
 * @return the setContPackUom
 */
public Set<String> getSetContPackUom() {
    return Collections.unmodifiableSet(setContPackUom);
}
/**
 * @param setContPackUom the setContPackUom to set
 */
public void setSetContPackUom(Set<String> setContPackUom) {
    this.setContPackUom.addAll(setContPackUom);
}
/**
 * @return the setVol
 */
public Set<Number> getSetVol() {
    return Collections.unmodifiableSet(setVol);
}
/**
 * @param setVol the setVol to set
 */
public void setSetVol(Set<Number> setVol) {
    this.setVol.addAll(setVol);
}
}

this looks good, before further to promote my changes into Production. Is there any other discrepancy ?

[Duc Vo]

All your setSet methods should rename to addAllSet methods, since they don't adhere to the standard of Java bean contract, hence they will mislead other developers and even you after sometime. If you insist to have the setSet methods, then you should have both methods and they should look like below

/** 
* @param setContPackQty the setContPackQty to set 
*/  
public void setSetContPackQty(Set<Number> setContPackQty) {  
    this.setContPackQty.clear();
    this.setContPackQty.addAll(setContPackQty);  
}  

public void addAllSetContPackQty(Set<Number> setContPackQty) {  
    this.setContPackQty.addAll(setContPackQty);  
}

Then your client code should use the second method if required to append to the existing set.
If you want to replace the existing set with a new set then the first method should be used.

[Amandeep Singh]

when i need to add Strings to existing Set, then instead of making a String and make into Set then adding, i prefered this way of doing-

public void checkAddContPackTypeStringToSetType(String ent_cont_pack_type) {  
	this.setContPackType.add(ent_cont_pack_type);
}

[Duc Vo]

Perfect, now you got the hang of it

[Amandeep Singh]

thank you for your help..