Elements in list are getting overwritten when adding to list

Question

I'm reading in tags and values from an XML file for information that is needed for a functional test software. When adding the Frequencies to the list, the frequencies for all other existing list items are overwritten by the new frequencies.

public struct Configuration 
{
    public string Description;
    public byte Mode;
    public byte Deviation;
    public string PartNumber;
    public UInt16[] Frequencies;
    public byte TxPower;
    public byte PWM;
    public Int16 HeaterSetpoint;
    public string FirmwareVersion;
    public bool Active;
    public int Threshold;
} // struct Configuration

Configurations = new List<Configuration>();
ushort[] Frequencies = new ushort [7];
Configuration TestConfig = new Configuration();

/*Open the XML file, load all the tag values into XmlNodeList */
//Pull in all the XML Tag Values Sorted by Tag Name
XmlNodeList xml_description = ConfigFile.GetElementsByTagName("description");
XmlNodeList xml_mode = ConfigFile.GetElementsByTagName("mode");
XmlNodeList xml_deviation = ConfigFile.GetElementsByTagName("deviation");
XmlNodeList xml_partnumber = ConfigFile.GetElementsByTagName("partnumber");
XmlNodeList xml_channel0 = ConfigFile.GetElementsByTagName("channel0");
XmlNodeList xml_channel1 = ConfigFile.GetElementsByTagName("channel1");
XmlNodeList xml_channel2 = ConfigFile.GetElementsByTagName("channel2");
XmlNodeList xml_channel3 = ConfigFile.GetElementsByTagName("channel3");
XmlNodeList xml_channel4 = ConfigFile.GetElementsByTagName("channel4");
XmlNodeList xml_channel5 = ConfigFile.GetElementsByTagName("channel5");
XmlNodeList xml_channel6 = ConfigFile.GetElementsByTagName("channel6");
XmlNodeList xml_txpower = ConfigFile.GetElementsByTagName("txpower");
XmlNodeList xml_pwm = ConfigFile.GetElementsByTagName("pwm");
XmlNodeList xml_hsp = ConfigFile.GetElementsByTagName("hsp");
XmlNodeList xml_firmware = ConfigFile.GetElementsByTagName("firmware");
XmlNodeList xml_active = ConfigFile.GetElementsByTagName("active");
XmlNodeList xml_threshold = ConfigFile.GetElementsByTagName("threshold");

/*Use LINQ to check that the XmlNodeLists are all the same length. This way we don't try and load invalid data*/

if(!listsaresamelength)
{
    /*Alert user there is a problem with the XML file and close the application*/
}

//Loop to add values to the List
for(i = 0; i < NumberofXmlValues; i++)
{
    /*check that the description, Mode, Deviation are valid and add them to DummyConfig*/

    try
    {
        Frequencies[0] = UInt16.Parse(xml_channel0[i].InnerText);
        Frequencies[1] = UInt16.Parse(xml_channel1[i].InnerText);
        Frequencies[2] = UInt16.Parse(xml_channel2[i].InnerText);
        Frequencies[3] = UInt16.Parse(xml_channel3[i].InnerText);
        Frequencies[4] = UInt16.Parse(xml_channel4[i].InnerText);
        Frequencies[5] = UInt16.Parse(xml_channel5[i].InnerText);
        Frequencies[6] = UInt16.Parse(xml_channel6[i].InnerText);
        //Move the frequencies to the list
        TestConfig.Frequencies = Frequencies;
    }

    catch 
    {
        /*Problem with the XML file. Alert the user and close the application*/
    }

    /*check that the heater set point, PWM set point, partnumber, Txpower, and Threshold are valid and add them to DummyConfig*/

    Configurations.Add(TestConfig)

}

The first iteration of the for loop works fine; here's the result: first loop iteration second loop iteration

I've tried a few different ways to change how I'm writing to the list, but they all generate the same result. I feel like I'm missing something obvious.

"When adding the Frequencies to the list" - you're adding `DummyConfig` to the list, but we have no idea what that is. You're not using it anywhere in the rest of the loop. But additionally, you're using the same array on every iteration of the loop - you never create a new array. — Jon Skeet, May 12 '20 at 18:20
I mistyped that one. I'll try making the new array. Just thinking about that though, that seems kinda clunky. Would there be a nicer solution? If it matters, I'm also picking this up where someone else left off. I've broken a lot of this existing code and had to fix a lot of it. — Billy, May 12 '20 at 18:25
If you want references to several different, separate, independent arrays in the list, you need to create one array per iteration of the loop. I don't see what's clunky about that. — Jon Skeet, May 12 '20 at 18:29
I wasn't thinking about doing in each iteration of the loop, but creating them outside the loop. Makes a lot more sense. Thanks for your comments. — Billy, May 12 '20 at 18:32

StriplingWarrior · Accepted Answer · 2020-05-12T18:35:58.673

2

You need to move your TestConfig and Frequencies variables' instantiation into the for loop so that you're working with a different object each time.

for(i = 0; i < NumberofXmlValues; i++)
{
    TestConfig = new Configuration();
    Frequencies = new ushort [7];

I'd personally recommend making these locally-declared variables rather than persistent fields. That pattern will help you avoid mistakes like this in the future.

for(i = 0; i < NumberofXmlValues; i++)
{
    /*check that the description, Mode, Deviation are valid*/
    ...
    try
    {
        var testConfig = new Config
        {
            Description = description
            ...
            Frequencies = new [] {xml_channel0, xml_channel1, ...}
               .Select(c => UInt16.Parse(c[i].InnerText))
               .ToArray()
        };
        Configurations.Add(testConfig);
    }
    ...

edited May 12 '20 at 18:35

answered May 12 '20 at 18:26

StriplingWarrior

151,543
27
246
315

Thanks! The first snippet fixes my problem. Thank you for that. Your comment on locally-declared variables vs persistent fields, can you expand on that? Like why it's preferred, generic methods to do it, etc. C# is my first experience in object-oriented programming. I only learned C in school and everything I've learned about object-oriented programming has been in my professional career. – Billy May 12 '20 at 18:41
@Billy: There are plenty of discussions and articles about that principle. E.g. [here](https://stackoverflow.com/q/1411463/120955), [here](https://softwareengineering.stackexchange.com/q/113262/7866), [here](https://wiki.c2.com/?DeclareVariablesAtFirstUse). Also, [_Code Complete_ recommends](https://books.google.com/books?id=pDsFCAAAQBAJ&lpg=PA257&ots=RmNxIEzzUV&dq=declare%20variables%20close%20to%20where%20they%20are%20used&pg=PA241#v=onepage&q=declare%20variables%20close%20to%20where%20they%20are%20used&f=false) to "initialize each variable close to where it's first used." – StriplingWarrior May 12 '20 at 21:35

Elements in list are getting overwritten when adding to list

1 Answers1