java arraylist.add overwrites with last value

Question

have this class

public class ObjetoOS {

private ArrayList<String> atribute = new ArrayList<String>();

public ObjetoOS (String a){
    atribute.add(a);
}

public ObjetoOS (ArrayList<String> a){
    atribute = a;
    }

which i use like this

public class TablaSimbolica {

    private static ArrayList<ObjetoOS> tableOS = null;

    public static void addAtributos(ArrayList<String> newOnes){

    if (tableOS == null){
        tableOS = new ArrayList<ObjetoOS>();
        for (String s : newOnes){
            ObjetoOS newObj = new ObjetoOS(s);
            tableOS.add(newObj);
        }
    }
    else{
        ArrayList<ObjetoOS> aux = new ArrayList<ObjetoOS>();
        for (ObjetoOS os : tableOS){
            ArrayList<String> oldOnes = new ArrayList<String>();
            oldOnes = os.getAtributo();
            for (String s : newOnes){
                oldOnes.add(s);
                ObjetoOS newObj = new ObjetoOS(oldOnes);
                aux.add(newObj);
                newObj = null;
                oldOnes.remove(s);
            }
        }   
        tableOS = aux;
    }

}

so basically: addAtributos checks if the array its empty. if it is it just adds the strings, if its not, i have to combine it with the new ones, like a cartisan product thing.

when adding new strings, although im creating a new array, and a new objectOS which y turn to null after adding it, the elements overwrite with the last one.

for example, if i had strings "true" "false", and i have to add "female" "male", the output is:

[TRUE, Female]

[TRUE, Male]
[TRUE, Male]

[TRUE]
[TRUE]
[FALSE, Female]

[TRUE]
[TRUE]
[FALSE, Male]
[FALSE, Male]

i cant figure out where im missing the point here. it gives me an error if i erase the static both from the ArrayList tablaOS or from the addAtributos method.

EDIT: solved! i changed the object class making a new empty constructor and a method adding a string array with a for.

the loop it ended up being

for (String s : newOnes){
    oldOnes.add(s);
    ObjetoOS nuevo = new ObjetoOS();
    nuevo.addAtribute(oldOnes);
    aux.add(nuevo);
    oldOnes.remove(s);

It is somewhat difficult for programmers working in English to mentally deal with variable names in a different language. Can you refactor your code to have variables that are appropriately-named in English? — nanofarad, May 08 '14 at 00:48

score 1 · Accepted Answer · edited May 23 '17 at 11:50

Some of your problems lies here:

ArrayList<String> oldOnes= new ArrayList<String>(); // This line is pointless
oldOnes= os.getAtributo();                          // oldOnes will be overwritten here
    for (String s : newOnes)
    {
       oldOnes.add(s);
       ObjetoOS newObj = new ObjetoOS(oldOnes);
       aux.add(newObj );
       newObj = null;
       oldOnes.remove(s);
    }

When you here pass oldOnes to the ObjetoOS constructor, the newObj will have a reference to the same list as oldOnes, when you then later remove the element from oldOnes you also remove it from newObj's list(since they are the same).

The same does not happen if you use the String constructor because primitive datatypes, also including Strings, are copied (passed by value) when passed to a method.

There's a good explanation on how objects are passed in java here: Is Java "pass-by-reference" or "pass-by-value"?

Edit: Also your tableOS will point to a new ArrayList<> every time you call your method, if I understand your solution correctly this might be more what you want:

...
    for (ObjetoOS os : tableOS)
    {
        for (String s : newOnes)
        {
            ArrayList<String> oldOnesPlusNewOne = new ArrayList<>();
            oldOnesPlusNewOne.addAll(os.getAtributo());
            oldOnesPlusNewOne.add(s);
            ObjetoOS newObj = new ObjetoOS(oldOnesPlusNewOne);
            tableOS.add(newObj);
        }
    }

solved! i changed the object class making a new empty constructor and a method adding a string array with a for. the loop it ended up being for (String s : newOnes){ oldOnes.add(s); ObjetoOS nuevo = new ObjetoOS(); nuevo.addAtribute(oldOnes); aux.add(nuevo); oldOnes.remove(s); — user3583107, May 08 '14 at 01:27

score 1 · Answer 2 · answered May 08 '14 at 01:29

Might I suggest simplifying the code some? If your trying to add attributes to an array list, maybe create an attributes class instead. You can create a class for the atributos, give it some members etc... and easily change to and make the atributos a list to hold aritbutos. I'd suggest the class be name Atributo. Your trying to do to much in one class. Single responsibility is key. One class that defines the atributo and one to store a list of them. This should fix your problem and uncomplicate your code. Now you can only store 10 members to the array list without specifying the amount of elements you want it to hold. If you don't it will only hold the default of 10 and the last element will be over written.

import java.util.ArrayList;

public class Atributos 
{
    // attributes list
    ArrayList<String> atributos = new ArrayList<String>();

    // add string to attributes list
    public void addAtributos(String atributo)
    {
        atributos.add(atributo);
    }

    // add list of strings to attribute list
    public void addAtributos(ArrayList<String> atributo)
    {
        for(String attr : atributo)
        {
            atributos.add(attr);
        }
    }
}

welcome, ArrayList atributos = new ArrayList(); needs to be ArrayList atributos = new ArrayList<>(); or you'll get a warning on the datatype. It won't break your code but it won't flag a warning. — user3587554, May 08 '14 at 19:36

java arraylist.add overwrites with last value

2 Answers2