Go weird behaviour - variable not incrementing correctly

Question

I have the following code that adds a new element to a slice if it doesnt exist already. If it does exist then the qty property should be incremented of the existing element instead of a new element being added:

package main

import (
    "fmt"
)

type BoxItem struct {
    Id int
    Qty int
}

type Box struct {
    BoxItems []BoxItem
}

func (box *Box) AddBoxItem(boxItem BoxItem) BoxItem {

    // If the item exists already then increment its qty
    for _, item := range box.BoxItems {
        if item.Id == boxItem.Id {
             item.Qty++
             return item
        }
    }

    // New item so append
    box.BoxItems = append(box.BoxItems, boxItem)
    return boxItem
}


func main() {

    boxItems := []BoxItem{}
    box := Box{boxItems}

    boxItem := BoxItem{Id: 1, Qty: 1}

    // Add this item 3 times its qty should be increased to 3 afterwards
    box.AddBoxItem(boxItem)
    box.AddBoxItem(boxItem)
    box.AddBoxItem(boxItem)


    fmt.Println(len(box.BoxItems))  // Prints 1 which is correct

    for _, item := range box.BoxItems {
        fmt.Println(item.Qty)  // Prints 1 when it should print 3
    }
}

The problem is that the qty is never incremented correctly. It always ends in 1 when it should be 3 in the example provided.

I have debugged the code and it does appear that the increment section is reached but the value just isnt persisted to the item.

What is wrong here?

Answer 1

You are incrementing Qty in the copy of the box.BoxItems because range will yield the copy of the elements in the slice. See this example.

So, in for _, item := range box.BoxItems, item is a copy of of the elements in box.BoxItems.

Change your loop to

for i := 0; i < len(box.BoxItems); i++ {
    if box.boxItems[i].Id == boxItem.Id {
         box.boxItems[i].Qty++
         return box.BoxItems[i]
    }
}

Playground

Answer 2

I will answer your question pretty much like others have done. However, not that the problem you try to solve is not best served by looping over a range of values. Read on:

Solution to your question

Like others have said, for-range provide an immutable iteration over the range of values. That means any change you make to the value provided in the iteration will be lost. It's basically giving you a copy of the real value, not the actual value.

for _, item := range box.BoxItems {
//     ^-not the real `item`, it's a copy!

A way around this is to keep track of the indexing value in the for idx, val := range, and use this idx to address the value you look for directly.

If you change your for-loop to keep the index value:

for i, item := range box.BoxItems {
//  ^-keep this

You will be able to reference the actual item in the array you loop on:

for i, item := range box.BoxItems {
    // Here, item is a copy of the value at box.BoxItems[i]
    if item.Id == boxItem.Id {
         // Refer directly to an item inside the slice
         box.BoxItems[i].Qty++
         return box.BoxItems[i] // Need to return the actual one, not the copy
    }
}

Playground

I would favor this approach over the for i; i<Len; i++ one as I find it more readable. But this is simply a matter of taste and the for i form will be more efficient (beware of premature-optimization!).

Your real problem is

What you're trying to do is to avoid duplicating BoxItems if their Id already exists. To do this, you iterate over the whole range of the box.BoxItems slice. If you have N items in your box.BoxItems slice, you will potentially iterate over all N items before finding out that the item you're looking for doesn't exist! Basically, this means your algorithm is O(N).

If you increment Id in natural order

That is, 0, 1, 2, 3, ..., n - 1, n, you can keep using a slice to index your box items. You would do like this:

func (box *Box) AddBoxItem(boxItem BoxItem) BoxItem {
    // Lookup your item by Id
    if boxItem.Id < len(box.BoxItems) {
        // It exists, do don't create it, just increment
        item := box.BoxItems[boxItem.Id]
        item.Qty++
        box.BoxItems[boxItem.Id] = item
        return item
    }
    // New item so append
    box.BoxItems = append(box.BoxItems, boxItem)
    return boxItem
}

Playground

If you increment Id in any order

You should use a datastructure that offers fast lookups, such as the built-in map, which offers O(1) lookups (that means, you need to do a single operation to find your item, not n operations).

type Box struct {
    BoxItems map[int]BoxItem
}

func (box *Box) AddBoxItem(boxItem BoxItem) BoxItem {

    // Lookup the map by Id
    item, ok := box.BoxItems[boxItem.Id]
    if ok {
        // It exists, do don't create it, just increment
        item.Qty++
    } else {
        item = boxItem
    }

    // New item so add it to the map
    box.BoxItems[boxItem.Id] = item
    return item
}

Playground

This is a more correct way to solve your problem.

Answer 3

In the index, value := range someSlice, the value is a fresh new copy of someSlice[index].

package main

import (
        "fmt"
)

type BoxItem struct {
        Id  int
        Qty int
}

type Box struct {
        BoxItems []BoxItem
}

func (box *Box) AddBoxItem(boxItem BoxItem) BoxItem {

        // If the item exists already then increment its qty
        for i := range box.BoxItems {
                item := &box.BoxItems[i]
                if item.Id == boxItem.Id {
                        item.Qty++
                        return *item
                }
        }

        // New item so append
        box.BoxItems = append(box.BoxItems, boxItem)
        return boxItem
}

func main() {

        boxItems := []BoxItem{}
        box := Box{boxItems}

        boxItem := BoxItem{Id: 1, Qty: 1}

        // Add this item 3 times its qty should be increased to 3 afterwards
        box.AddBoxItem(boxItem)
        box.AddBoxItem(boxItem)
        box.AddBoxItem(boxItem)

        fmt.Println(len(box.BoxItems)) // Prints 1 which is correct

        for _, item := range box.BoxItems {
                fmt.Println(item.Qty) // Prints 1 when it should print 3
        }
}

Playground

---

Output:

1
3