Range over slice (element vs index)

Getting Help Code Review
1

Hello all,

I’m pretty new to Go, just started learning it 6 weeks ago. I’m coming from using Python for the last 3 years, and am 100% self-taught (my ADHD makes it exceptionally hard to take formal classes or read a book cover to cover.

Anyways, I am building my first Go project for work and have come across what may be a fairly trivial issue, but I haven’t been able to find a post on this specific question.

When ranging over a slice of structs, is it more efficient to range over the element or the index?

To start, here are my project requirements and the approach I’m taking:
I work for a nationwide retailer and am building a script that puts stores in like groups, based on the last full quarter of sales data. Comparing total sale volume, top items sold, source warehouse and relative location.

To do this, I have built a series of structs and typed slices of those structs. (I’ve redacted some of the code due to company IP rules)

Below is some of my code along with two functions that demonstrate the two approaches.

In addition to thoughts on which approach is more memory efficient, any general feedback on my code would also be greatly appreciated! I’m too far in to this project to make significant changes, but I will definitely take note for application in future projects.

type storeMaster struct {
	store      int
	location   storeLocation
	department storeDepartment
}

type storeLocation struct {
	warehouse int
	postal int
}

type storeDepartment struct {
	department int        //department id (for grouping stores at a department level)
	weeks      int        //count of weeks with active sales
	retail     float64    // $$
	avgRetail  float64    // $$ retail / float64(weeks)
	lastWeek   int        //most recent week with sales
	items      storeItemSlice 
}

type storeItem struct {
	upc       int64
	weeks     int        //count of weeks with active sales for item
	retail    float64    // $$
	avgRetail float64    // $$ retail / float64(weeks)
	lastWeek  int        //most recent week with sales for item
}

type storeMasterSlice []storeMaster
type storeItemSlice []storeItem

func (u storeItemSlice) Len() int           { return len(u) }
func (u storeItemSlice) Less(i, j int) bool { return u[i].avgRetail > u[j].avgRetail }
func (u storeItemSlice) Swap(i, j int)      { u[i], u[j] = u[j], u[i] }
func (u storeItemSlice) Sort()              { sort.Sort(u) }

// function that ranges over the elements
func (m *storeMaster) topItems() (top10, top25, top50 []int64) {
	var items storeItemSlice

	weeks, lastWeek := (*m).maxWeek()

	weeks = int(math.Round(float64(weeks) * .75))
	lastWeek = lastWeek - 3

	for _, item := range (*m).department.items {
		if item.weeks >= weeks && item.lastWeek >= lastWeek {
			items = append(items, item)
		}
	}

	items.Sort()

	for i := 0; i < 50; i++ {
		top50 = append(top50, items[i].upc)
	}
	top25 = top50[:25]
	top10 = top50[:10]

	return top10, top25, top50
}

// function that ranges over the index
func (m *storeMaster) maxWeek() (weeks, lastWeek int) {
	for i := range (*m).department.items {
		if (*m).department.items[i].weeks > weeks {
			weeks = *m.department.items[i].weeks
		}
		if (*m).department.items[i].lastWeek > lastWeek {
			lastWeek = (*m).department.items[i].lastWeek
		}
	}

	return weeks, lastWeek
}

Thank you in advance for any feedback you can give me! :slight_smile:

2

It’s depends. Golang doesn’t have range form for references on elements so all values are copied. So:

  1. Use elements if you don’t want modify them and their size is small.
  2. Use indexes (or pointer x := &data[i]) otherwise.
1 Like
3

As for code. It would be better to have minimal but complete code, but:

  1. What if there are less than 50 elements?

  2. You should always pre-allocate memory when it possible.

  3. Don’t use pointer receiver on slice that doesn’t reslice or reallocate the slice. Choosing ReceiverType recommendations.

1 Like
4

And, as always, start with the clearest and most straightforward code. If you later identify it as a performance problem, by measuring and profiling, then it’s time to consider the alternatives.

3 Likes
5

Thank you both for the feedback! Especially for the link to the code review comments article, that has been added to my favorites.

@GreyShatter
If you don’t mind though, I had a couple of follow up questions.

I know that there is no set number, but what would be a good rule of thumb for deciding if it is small? I used unsafe.SizeOf on several of my storeItemSlices and the average was 28K (~700 items).

Would this be done by using an array with a set capacity?

No question here, just wanted to thank you for that. At a department level our smallest stores wont drop below ~500 distinct items sold in the time periods we are looking at. However this is something I had not thought about if I get asked to apply this to small sub-department or category levels. I will add a size check before trying to populate the “top” slices.

6

I also wanted to specifically thank you for that reminder. I do tend to get caught up in the what ifs a little too much

1 Like
7

Developers of go-critic set the value as 128 bytes on iteration for such check.

I meant provide initial size for slice:
top50 := make([]int64, 50)
and so on.

1 Like
9

Ah, ok, I must have missed that part when learning about make(), that makes sense. Also, I’ve added the go-critic to my favorites list. Thank you!

10

Unlike C, the ‘.’ operator deferences pointers when necessary.
So (*m).department.items
is ususally written m.department.items

So I would write the function

func (m *storeMaster) maxWeek() (weeks, lastWeek int) {
	for i , w := range m.department.items {
		if w.weeks > weeks {
			weeks = w
		}
		if w.lastWeek > lastWeek {
			lastWeek = w.lastWeek
		}
	}

	return weeks, lastWeek
}
1 Like
11

I think I started doing that because another function early on kept throwing an error until I put parenthesis around the pointer, so I just started doing that for all my passed pointers, not sure why I never tried just taking off the *.

Thank you, I’ll remember that! :slight_smile: