CODEWITHSUNDEEP
c#loopsforeach
Christian Frantz
Christian Frantz

Reputation: 253

Order of operations in a foreach loop

I completely changed the way my crafting table works and I've run into a problem. The way it works now is it displays all the item in the crafting list. If you can't make the item, the item texture in the table is translucent.

My problem is that when I am checking if the item can be crafted, I am setting a bool variable CanCraft to true or false and it is not setting correctly. 

 public void CheckForAvailableCrafts(Player player)
    {
        foreach (ItemRecipe recipe in CraftingList)
        {
            if (recipe.CheckForItemsNeeded(player) != null)
            {
                Output = recipe.Output;
                UpdateTable(Output);
            }
            else if (recipe.CheckForItemsNeeded(player) == null)
            {
                Output = new Item();
                Output.ItemName = "empty";
                UpdateTable(Output);
            }

The CheckForItemsNeeded method works correctly because it worked fine before I rewrote this part of code. If I have a certain amount of items for the recipe CanCraft for the item is true. The problem lies within the UpdateTable method.

            private void UpdateTable(Item output)
    {
        foreach (Item item in CraftingTableItems)
        {
            if (output.ItemName == item.ItemName)
            {
                item.CanCraft = true;
                if (output.ItemName == "empty")
                {
                    item.CanCraft = false;
                }
            }
        }
    }

My CraftingTableItems is a list of 12 items, and each item is set to a recipe in my CraftingList. So I have 6 items in the CraftingTable that aren't empty.

After I've crafted the item, the item in the table should return to a false value, but it doesn't. Is the order of operations in my for loop correct?

Upvotes: 0

Views: 165

Answers (2)

Joel Coehoorn
Joel Coehoorn

Reputation: 415735

I suspect the problem is the Output variable. It's not defined as part of the method, and therefore could be modified by other code from another thread. You could change the code to make that variable local to the method (general rule of thumb: always use the smallest scope possible for a variable) or even eliminate it entirely, but I'd re-write the whole thing like this:

public void CheckForAvailableCrafts(Player player)
{
    //names of items the player can craft
    var names = CraftingList.Where(r => r.CheckForItemsNeeded(player) != null)
                            .Select(r => r.Output.ItemName);

    //Item objects that player can craft
    var items = CraftingTableItems.Join(names, i => i.ItemName, n => n, (i, n) => i);

    //Set all to false (base state)
    foreach (var item in CraftingTableItems) {item.CanCraft = false;}
    //Set items player can craft back to true
    foreach(var item in items) {item.CanCraft = true;}
}

Not only is that less code, but it should be much more efficient, as the prior code had to check every CraftingList entry for every possible item the player could craft... O(n2), while this is (I think) O(n Log n), maybe even close to O(n).

Yes, that .Join() call looks like greek if you're not used to lambda expressions, but it's really not that hard, and again: it greatly simplifies and speeds your code. You can simplify this further if you have an EqualityComparer class the compares two items based on the name, or if your Item type implements IEquatable to compare on ItemNames (and you may find that useful anyway). Assuming the latter, where Item correctly implements IEquatable, your code would look like this:

public void CheckForAvailableCrafts(Player player)
{
    //items the player can craft
    var craftableItems = CraftingList.Where(r => r.CheckForItemsNeeded(player) != null)
                                     .Select(r => r.Output);

    //Items objects from the CraftingTable that the player can craft
    craftableItems = CraftingTableItems.Intersect(craftableItems);

    //Set all to false (base state)
    foreach (var item in CraftingTableItems) {item.CanCraft = false;}
    //Set items player can craft back to true
    foreach(var item in craftableItems) {item.CanCraft = true;}
}

If you can't spot the difference at first, the first statement saves a step in the Select projection, and the second statement changes from the ugly Join() function to the easier-to-read Intersect()

Upvotes: 2

ΩmegaMan
ΩmegaMan

Reputation: 31616

Shot in the dark here, but I would do the following check in case someone has named something slightly different:

output.ItemName.ToLower() == item.ItemName.ToLower()

Upvotes: 0

Related Questions