views:

154

answers:

7

I've got a bit of code that I've been working on for a friend for the past few days. At a high level it parses a text file and writes to an MDB. To cut a long story short, I've got a nested couple of loops doing some processing on the items. The inner loop only gets called in certain cases, but when it does, it's doing some strange things.

ArrayList CaseRecordItems = new ArrayList(); // this is created earlier
string baseTif = "sometext_"; // this is created earlier
CaseRecord cr = new CaseRecord(); (this gets populated with "stuff")
char increment = 'A';

for (int i = 0; i < FirstNames.Count; i++)
{
    cr.Firstname = (string)FirstNames[i];
    cr.Lastname = (string)LastNames[i];
    if (FirstNames.Count > 1)
    {
        cr.Tif = baseTif + increment.ToString();
        increment++;
    }
    CaseRecordItems.Add(cr);
}

The loop runs for example two times and should set the value of cr.Tif to sometext_A and sometext_B. This works correctly, but once the second item is added to the collection, the value of the first is changed to match it.

I suspect this is due to a lack of my understanding how these objects are being instantiated and passed around. Any insight would be appreciated.

EDIT:

Based on the awesome feedback (and my numb-nutzery) the issue has been resolved. Thanks to Dan's answer I made a couple of changes to the code that I had tried before utilizing the clone function (yes, beach I had actually tried that :P).

The new block looks like this: ArrayList CaseRecordItems = new ArrayList(); // this is created earlier string baseTif = "sometext_"; // this is created earlier CaseRecord cr = new CaseRecord(); // this gets populated with "stuff") char increment = 'A';

for (int i = 0; i < FirstNames.Count; i++)
{
    CaseRecord cr2 = new CaseRecord();
    cr2 = cr.Clone();                        // preserves the data from outside
    cr2.Firstname = (string)FirstNames[i];
    cr2.Lastname = (string)LastNames[i];
    if (FirstNames.Count > 1)
    {
        cr2.Tif = baseTif + increment.ToString();
        increment++;
    }
    CaseRecordItems.Add(cr2);
}

Thanks everyone for the super-quick responses!

+6  A: 

I assume cr is an object. You're not creating a new cr each time, and so you have the same reference in the arraylist twice, thus when you modify it the second time you're actually working on the same object.

Dan Fuller
A: 

Since cr is not getting a new statement (I dont' know cr's type or I would show you), you are just adding the same reference over and over. If you want to add new cr items to CaseRecordItems you new to do a cr = new TypeOfCR(); otherwise you are just overwriting the same object over and over.

EDIT: Be sure to do the new inside of your loop

Greg
A: 

If you're not creating cr (whatever it is) inside the for loop, then you're modifying the same object, over and over, each time through the loop. You want to create a new cr instance inside the for loop.

Michael Petrotta
+1  A: 

You are changing the value of the cr instance inside the loop. Each iteration through the loop is using the same instance of cr so when you change it, they all change. To resolve this, an appropriate method is to use a local instance inside the loop:

for (int i = 0; i < FirstNames.Count; i++)
{
    CaseRecord cr=new CaseRecord();

    ...

    CaseRecordItems.Add(cr);
}
1800 INFORMATION
btw... good guess on the class name. you had that in here before I fixed the question text... hehe.
kdmurray
+1  A: 

The problem is that the variable cr is the same both times through the loop, so both times through the loop the same object is being modified and the same object is getting added to the ArrayList twice.

You don't show enough of the code to show where cr is declared.

Do you need to do something like

for (int i = 0; i < FirstNames.Count; i++)
{
    CRObject cr = new CRObject();

    cr.Firstname = (string)FirstNames[i];
    cr.Lastname = (string)LastNames[i];
    if (FirstNames.Count > 1)
    {
        cr.Tif = baseTif + increment.ToString();
        increment++;
    }
    CaseRecordItems.Add(cr);
}
David Norman
A: 

Assuming cr is a class, you will need to create a new instance of the class on each pass through the loop, like cr = new crClassName();

The reason is that what is being added to CaseRecordItems is a pointer to the object, not a copy of it - which is why when you change it on the second pass the first value appears to change also.

Bob
+1  A: 

Does the cr object have a clone function?

If so, this should do it:

CaseRecordItems.Add(cr.Clone());
beach