Java Bug?

[BlackDragon777]

Hi. I need to copy an entire object and not just the reference. Obviously the clone method first comes to mind. However in my case, it is not an option because my object is composed of multiple array lists. Again, I want to duplicate the entire object, including the values of its array lists. So I wrote a copy function. The following is my code.

	public static ArrayList<Content> copy(ArrayList<Content> a)
	{
		ArrayList<Content> b = new ArrayList<Content>();
		Content strTemp = new Content();
		ContentAttribute strTempAttr = new ContentAttribute();
		int z = a.size();
		for(int i=0; i<a.size(); i++)
		{
			b.add(strTemp);
			b.get(i).strName = a.get(i).strName;
			b.get(i).strGuiObject = a.get(i).strGuiObject;
			b.get(i).strCategory = a.get(i).strCategory;
			
			//System.out.println("SIZE: " + a.get(i).lstAttributes.size());
			int k = a.get(i).lstAttributes.size();
			b.get(i).lstAttributes = new ArrayList<ContentAttribute>();
			
			System.out.println("SIZE: " + b.get(i).lstAttributes.size());
			
			for(int j=0; j<a.get(i).lstAttributes.size(); j++)
			{
				b.get(i).lstAttributes.add(new ContentAttribute());
				//b.get(i).lstAttributes.set(new ContentAttribute());
				strTempAttr.strTagName = a.get(i).lstAttributes.get(j).strTagName;
				strTempAttr.strTagValue = a.get(i).lstAttributes.get(j).strTagValue;
				strTempAttr.strDefault = a.get(i).lstAttributes.get(j).strDefault;
				strTempAttr.strCurrentValue = a.get(i).lstAttributes.get(j).strCurrentValue;
				strTempAttr.jComponent = a.get(i).lstAttributes.get(j).jComponent;
				strTempAttr.jContentComponent = a.get(i).lstAttributes.get(j).jContentComponent;
				
				b.get(i).lstAttributes.set(j,strTempAttr);
			}
			//b.get(i).lstAttributes.add(null);
		}
		
		return b;
	}

However whenever I execute this, it takes all the lstAttributes sizes and totals them. It is as if the i variable in the first for loop is never incremented. I think logically my code is sound. Any ideas?

[petes1234]

A couple of questions.

1) Why can't you use clone? If what you are doing is copying objects, it seems to me that you are cloning. ArrayLists shouldn't stop you from doing this. It just means that you can't do a shallow clone, that you have to do a deep one.

2) I'm not sure I fully understand your code since it is not independently compilable, but you seem to be adding the very same Content object, strTemp into your arrayList. think on this. Each arrayList item points to only one single Object. Then you retrieving this very same object with the get(i). I think that every time you change your b.get(i) you change strTemp and every copy of strTemp in the arrayList. Better to create a new strTemp within the loop, and rather than accessing it through get(i), access it directly and then add it to the array list.

[keang]

It sounds like what you want is a deep clone as opposed to the shallow clone you get from calling clone(). Check out this article

[petes1234]

Here's an analogous program:

import java.util.ArrayList;
import java.util.List;

public class Fubar3
{
    private List<Foo3> fooList = new ArrayList<Foo3>();

    // this is what you are doing
    private void fillList()
    {
        fooList.clear();
        Foo3 myFoo3 = new Foo3();
        for (int i = 0; i < 10; i++)
        {
            // you think that you're making a list of objects that hold the
            // ints 0, 1, 2, 3, 4, 5, 6, 7, 8, 9
            // but you're not!  Each item in the list points to the same one object
            fooList.add(myFoo3);
            fooList.get(i).setMyInt(i);
        }
    }
    
    // this is what you should be doing
    private void betterFillList()
    {
        fooList.clear();
        for (int i = 0; i < 9; i++)
        {
            Foo3 myFoo3 = new Foo3();
            myFoo3.setMyInt(i);
            fooList.add(myFoo3);
        }
    }
    
    private void showList()
    {
        for (int i = 0; i < fooList.size(); i++)
        {
            System.out.println(fooList.get(i).getMyInt());
        }
    }
    
    public static void main(String[] args)
    {
        Fubar3 f3 = new Fubar3();
        System.out.println("fillList");
        f3.fillList();
        f3.showList();
        System.out.println();
        System.out.println("betterFillList");
        f3.betterFillList();
        f3.showList();
    }
    
    class Foo3
    {
        private int myInt = 0;
        
        public void setMyInt(int myInt)
        {
            this.myInt = myInt;
        }
        
        public int getMyInt()
        {
            return this.myInt;
        }
    }
}

[dlorde]

Hi. I need to copy an entire object and not just the reference. Obviously the clone method first comes to mind. However in my case, it is not an option because my object is composed of multiple array lists.

Having multiple array lists doesn't stop you using clone. You write the clone method so that it clones the lists, then iterates over the source list cloning each item and assigning it to the destination list, i.e. you clone the list contents yourself. This obviously means that the items in the list must be cloneable - and if they need to be cloned they should be made cloneable.

I think logically my code is sound ...

If that were the case, you wouldn't have the problem you've got. If code compiles and runs but doesn't do what you expect, then the syntax is clearly sound (it compiles), but the logic is clearly unsound (it does the wrong thing). So your logic has let you down twice

Seriously though, the code you have posted has a several problems, but the main one seems to be that it takes a list of items and tries to clone it by creating a new list, creating a single new item and adding this new item to the new list many times, changing its data each time. The result is a cloned list that contains references all pointing to the same object, which contains the data from the last item in the source list, which has overwritten all the previous values in the cloned item.

If you want to clone every item in a list, you have to create that many new items - one new item isn't enough.

If you are curious, I can explain how you can avoid errors like this by following a few very simple Object Oriented programming guidelines.

Object-oriented programming is an exceptionally bad idea which could only have originated in California..
E. Dijkstra

[dlorde]

Check out this article

The serialization cloning method described in that article is attractive at first glance, because it seems quick and simple - but by the time a version has been developed with reasonable performance, you have written new input and output stream classes, and on top of this, all the classes to be cloned must be serializable - and you may not want them to be serializable. Also, the reverse is true, which means you don't have control over what is cloned or how deep the clone is. If the members of a class are serializable, this method will include them in the copy - but you may not want them cloned.

It's a 'clever' workaround technique which will work in many cases, but it's poor practice. It can't be used if you need to distinguish between cloneability and serializability, and if it is used, you'll have problems if you find you subsequently do need to distinguish between them.

Cloneable and Serializable are separate interfaces for a reason - cloning and serializing are separate idioms. Java's clone method may not be perfect, but it's not that bad.

The competent programmer is fully aware of the strictly limited size of his own skull; therefore he approaches the programming task in full humility, and among other things he avoids clever tricks like the plague...
E. Dijkstra

[BlackDragon777]

keang, thatnks for you post. I think you provided the most useful information and I am researching it now.

As for petes and dlorde, I am sorry because I don't think I did a good job explaining my code.

First I am not a newbie. I have been coding for a long time. And I know how rare it is to find a bug in a programming language such as java that has been living for such a long time. However it is possible. Infact I was very apprehensious to even post it with that title. However the fact remains I still believe my code is logically sound. I will explain why.

I have a class called Content. Content has several values such as a name, and a category. It also has an arraylist of the class object ContentAttributes.
Inside ContentAttributes, I have several values such as a name and a value for the attribute.

I made an arraylist of the type class because I would have a lot of content. Now the problem I had was that my arraylist of content was static. I did this so I would only have one content database floating around in memory. However later I saw it was nessacary to duplicate this arraylist. As feang pointed out, the clone method on does a shallow copy. In other words, immediate values like the contents name and category are being cloned properly. However the arraylist to the contentAttributes are simply a bunch of references that are being copied. I didn't want that. I want the actual data AND the reference for those contentAttributes to be cloned.

So now let me explain the code.

	public static ArrayList<Content> copy(ArrayList<Content> a)
	{
		ArrayList<Content> b = new ArrayList<Content>();

// it does not matter that I have only one Content here
// because it is simply a reference that will get a new memory address
// each time I call new Content() which is precisely what i want.
// There is no need to create 200 physcial Content objects in the code.
// the for loop will do this just fine.
		Content strTemp = new Content();
		ContentAttribute strTempAttr = new ContentAttribute();


//  Lets go through each content and copy their immediate values
		for(int i=0; i<a.size(); i++)
		{
			b.add(strTemp);
			b.get(i).strName = a.get(i).strName;
			b.get(i).strGuiObject = a.get(i).strGuiObject;
			b.get(i).strCategory = a.get(i).strCategory;
			

//  Create memory on the heap for lstAttibutes for each content in the content arraylist.
			b.get(i).lstAttributes = new ArrayList<ContentAttribute>();
//  Copy each content attribute value to the newly created content attribute
			for(int j=0; j<a.get(i).lstAttributes.size(); j++)
			{
// Add a blank contentAttribute to the lstAttribute
				b.get(i).lstAttributes.add(new ContentAttribute());
				strTempAttr.strTagName = a.get(i).lstAttributes.get(j).strTagName;
				strTempAttr.strTagValue = a.get(i).lstAttributes.get(j).strTagValue;
				strTempAttr.strDefault = a.get(i).lstAttributes.get(j).strDefault;
				strTempAttr.strCurrentValue = a.get(i).lstAttributes.get(j).strCurrentValue;
				strTempAttr.jComponent = a.get(i).lstAttributes.get(j).jComponent;
				strTempAttr.jContentComponent = a.get(i).lstAttributes.get(j).jContentComponent;
				
// Set the values
				b.get(i).lstAttributes.set(j,strTempAttr);
			}
		}
		
		return b;
	}

So lets say, I have a content arraylist that only has 2 contents.
And the first content has 3 attributes.
The second Content has 7 attributes.

The problem is that my copied content arraylist would show up as having
2 contents as expected, but each of those contents would have a total of 10 attributes. That is not correct. I am not sure either because again, by following my code I believe my code is logically sound. Anythis I overlook?

[dlorde]

... Better to create a new strTemp within the loop, and rather than accessing it through get(i), access it directly and then add it to the array list.

Yes, absolutely, it shouldn't be added to the list until it is valid and complete, so whatever happens, the list only contains valid items (just as a constructor never returns an object if it can't complete).

There is nothing so useless as doing efficiently that which should not be done at all...
P. Drucker

[BlackDragon777]

Thanks pete dlorde, and keang. I followed pete's advice. here is my final code.

	public static ArrayList<Content> copy(ArrayList<Content> a)
	{
		ArrayList<Content> b = new ArrayList<Content>();
		Content strTemp = new Content();
		ContentAttribute strTempAttr = new ContentAttribute();

		for(int i=0; i<a.size(); i++)
		{
			strTemp = new Content();
			strTemp.strName = a.get(i).strName;
			strTemp.strGuiObject = a.get(i).strGuiObject;
			strTemp.strCategory = a.get(i).strCategory;
			b.add(strTemp);
			
			b.get(i).lstAttributes = new ArrayList<ContentAttribute>();
			
			for(int j=0; j<a.get(i).lstAttributes.size(); j++)
			{
				strTempAttr = new ContentAttribute();
				strTempAttr.strTagName = a.get(i).lstAttributes.get(j).strTagName;
				strTempAttr.strTagValue = a.get(i).lstAttributes.get(j).strTagValue;
				strTempAttr.strDefault = a.get(i).lstAttributes.get(j).strDefault;
				strTempAttr.strCurrentValue = a.get(i).lstAttributes.get(j).strCurrentValue;
				strTempAttr.jComponent = a.get(i).lstAttributes.get(j).jComponent;
				strTempAttr.jContentComponent = a.get(i).lstAttributes.get(j).jContentComponent;
				
				b.get(i).lstAttributes.add(strTempAttr);
			}
			//b.get(i).lstAttributes.add(null);
		}
		
		return b;
	}

This works correctly. Thanks

[dlorde]

As for petes and dlorde, I am sorry because I don't think I did a good job explaining my code.

Your explanation was comprehensible, and I examined the code to find out what the problem is. It's pretty simple, really.

First I am not a newbie. I have been coding for a long time. And I know how rare it is to find a bug in a programming language such as java that has been living for such a long time. However it is possible.

You may not be a newbie to coding, but, from the code you posted, you are either a newbie to Java or to OO programming. And if you know how unlikely it is to find a bug doing simple stuff like this in a well-established language, why on earth did you mention it - you just made a coding error, that's all - it happens to all of us.

However the fact remains I still believe my code is logically sound. I will explain why...

Believing it doesn't make it true - your code is logically unsound for the reasons I explained previously.

...
// it does not matter that I have only one Content here
// because it is simply a reference that will get a new memory address
// each time I call new Content() which is precisely what i want.
// There is no need to create 200 physcial Content objects in the code.
// the for loop will do this just fine.
...

This comment would match the code if you called new Content() more than once, but you don't. Just look through the code you posted and show me where new Content() is called inside the 'for' loop.

... by following my code I believe my code is logically sound. Anythis I overlook?

This isn't like politics, repeating it over and over it won't make it true

If you like, I can show you how to use the clone method to reduce your Content array list copy method to:

	public static ArrayList<Content> arrayCopy( ArrayList<Content> source) {
 
		ArrayList<Content> dest = new ArrayList<Content>();
 
		for (Content item : source) {
			dest.add((Content) item.clone());
		}
 
		return dest;
}

Furious activity is no substitute for understanding...
H.H Williams

[dlorde]

Apologies, my post came a bit late - but the offer still stands.

There is no reason anyone would want a computer in their home...
K. Olsen (Founder and President, Digital Equipment Corporation)

[BlackDragon777]

Yes dlorde, please teach me what you are doing there.

[dlorde]

OK - if you implement Cloneable in both the ContentAttribute and Content classes, it becomes simpler. The clone() method works just like any other overrideable method, with a couple of differences - the basic Object.clone() method is protected, so subclasses overriding it should make it public. Also, Object.clone() throws CloneNotSupportedException, so an immediate subclass method that overrides it should suppress the exception and not declare it.

Other than that, all you need to know is that calling super.clone() in an immediate subclass of Object returns an object of the correct size (memory allocation), with all the contents shallow copied (it's just a bitwise copy). This means that your clone method can call super.clone() to get a shallow copy, and then clone any member objects that need to be deep copied. You don't need to clone immutable objects like String, because they are, er, immutable

Applying all this to your Content and ContentAttribute classes, you get something like this (bear with me, I'm guessing what these classes actually contain):

public class Content implements Cloneable {
	...
 
	public Object clone() {
		Content theClone = null;
		try {
			theClone = (Content)super.clone(); // allocate memory, shallow copy fields
 
			// clone the list and replace its items with clones
			theClone.lstAttributes = lstAttributes.clone();
 
			for (int i=0; i < lstAttributes.size(); i++) {
				theClone.lstAttributes.add(i, (ContentAttribute) lstAttributes.get(i).clone());
			}
		}
		catch(CloneNotSupportedException cnse) { /* suppress - we do support clone */ }
 
		return theClone;
	}
} 
 
class ContentAttribute implements Cloneable {
	...
 
	public Object clone() {
		ContentAttribute theClone = null;
		try {
			theClone = (ContentAttribute) super.clone(); // allocate memory, shallow copy fields.
		}
		catch (CloneNotSupportedException e) {/* suppress - we do support clone */ }
 
		return theClone;
	}
}

I notice that the ContentAttribute class contains jComponent and jContentComponent fields. If these are Swing JComponent objects, they won't support cloning, so the default shallow copy will have to do - and you make a shallow copy of these in your code, so I assume it's OK.

That's it. Now if you call Content.clone(), you'll get a 'deep enough' copy which clones the ContentAttribute items in the lstAttributes list.

Any subclasses should use the same principle of calling the superclass.clone() method and then cloning any fields that need to be deep copied.

When implemented properly, clone can be simple and elegant, and it gives each class full control of how its own fields are copied.

The key to performance is elegance, not batallions of special cases...
J. Bently & D. McIlroy

[BlackDragon777]

Thanks for the tip dlorde.

[dlorde]

> May GOD Bless, Strengthen, Guide, Protect, and Reveal Wisdom to you Always!

Right... I don't do superstition myself, but if it really makes you feel better, invoke away