C# Changing Objects within a List

Question

Hi, I'm having a little problem changing members of an object in a list using a found index.

So this is the method I am currently working with:

static void addToInventory(ref List<baseItem> myArray, baseItem item, float maxAmount, ref float currentAmount)
{
    if (currentAmount + item.getWeight() <= maxAmount)
    {
        Console.WriteLine("item.Quantity = {0}", item.Quantity);
        if (myArray.Contains(item))
        {
            Console.WriteLine("Item ({0}) already exists", item.Name);
            int id = myArray.IndexOf(item);
            myArray[id].Quantity += item.Quantity;//Change occurs in this line, item.Quantity becomes the same as myArray[id].Quantity
        }
        else
        {
            Console.WriteLine("Adding new item ({0})", item.Name);
            myArray.Add(item);
        }
        currentAmount += item.getWeight();
    }
    else
    {
        Console.WriteLine("Inventory full");
    }
    myArray.Sort();
}

This method takes several parameters including the inventory/list. I check if the item fits in and if it does, I see if there is another item of the same name in the list, find the index, and add more of the item. However, the quantity of the item added suddenly becomes the same as the quantity of the item in the list. For some reason, this also changes the quantity of the item outside of the list. So therefore, instead of quantities adding up like this: 1, 2, 3, 4, they add up like this: 1, 2, 4, 8. How can I make it so that the quantity of the added item does not change?

I've just started to learn how to use lists so if there is anything I'm missing, don't hesitate to criticize. Thanks in advance.

To Mark: Thanks for the very quick reply! Sorry about the poor naming (myArray); it used to be an ArrayList. The currentAmount and the maxAmount refer to the current weight in the inventory and the max weight the inventory can hold respectively. Also, I don't want to just add 1 to the quantity; I want it to add the quantity of the item I pass in. Thanks for the tips. I'll probably look into using Dictionary instead.

Answer 1

What's happening here is that myArray[id] and item refer to the same object. List.Contains compares by reference, not by value.

So it looks like you want to simply do

if (myArray.Contains(item))
{
    item.Quantity++;
}

to indicate that there's one more of that item in the list.

However, using a list this way is a fundamentally incorrect approach. You should be using a Dictionary or a Set to achieve O(1) lookups.

If you took the dictionary route, you'd have something like:

// a Set might be better -- I don't know what you're using this for
var myDict = new Dictionary<string, BaseItem>();
// ...
if (currentAmount + item.Weight <= maxAmount)
{
    if (myDict.ContainsKey(item.Name))
    {
        Console.WriteLine("Item already exists");
        item.Quantity++;
    }
    else
    {
        Console.WriteLine("Adding new item");
        myDict[item.Name] = item;
    }

    currentAmount += item.Weight;
}
else
{
    Console.WriteLine("Full");
}

A few other notes:

• Avoid ref arguments (unless you know what you're doing). There's no reason to have the poorly named myArray (myList would be an improvement, but inventory would be spot-on) passed as a reference here.
• Prefer properties over getXXX() methods.
• Prefer classes over arbitrary static methods. The method you've described here sounds like it belongs to an Inventory class, not as some static method unrelated to a particular instance somewhere. ref float currentAmount should be a member of the class, not an explicit parameter to the method.
• Run StyleCop over your code. Make it a pre-build event. This will force you to get into good C# style habits, such as naming classes and methods with a capital letter (and using properties over get/set methods too).

Answer 2

You would be better using a Dictionary in which you use the item's name to index individual items.

In that sense, rather than searching for the Item (and keeping similar to your example), you can simply go

/* Remove ref on dictionary, you're not setting myArray to something else */

static void addToInventory(Dictionary<string, baseItem> myArray, baseItem item, float maxAmount, ref float currentAmount)  
{  
    if (currentAmount + item.getWeight() <= maxAmount)  
    {  
        Console.WriteLine("item.Quantity = {0}", item.Quantity);  

        if (myArray[item.Name] == null)
            Console.WriteLine("Adding new item ({0})", item.Name); 
        else
            Console.WriteLine("Item ({0}) already exists", item.Name); 

        myArray[item.Name] = myArray[item.Name] ?? item;
        myArray[item.Name].Quantity += item.Quantity;
        currentAmount += item.getWeight();  
    }  
    else  
    {  
        Console.WriteLine("Inventory full");  
    }
}  

I'd also be tempted to create your own Inventory class (which held its own Dictionary field) so you could do method chaining i.e.:

Inventory i = new Inventory(maxAmount);
float currentItemCount = i.add(item).add(item).add(item).getQuantity(item);

You'd achieve this by having your add method return 'this' (i.e. type Inventory not void).

getQuantity would just be 'return myArray[item.name].Quantity', except of course you shouldn't call the Dictionary variable within your Inventory class 'myArray'.

Answer 3

These lines are referring to the same item within the list

int id = myArray.IndexOf(item);
myArray[id].Quantity += item.Quantity;//Change occurs in this line, item.Quantity becomes the same as myArray[id].Quantity

Since item is already in the list you are getting another variable refering to the same object when you say IndexOf. It seems that you may have a bug in your logic earlier in your program because item will always have the same quantity of myArray[id].Quantity.

From your code snippet it seems that you want to create a new item and determine if that type of item is already in the list. If it is then you would add it's quantity to the pre-existing item of that type.