Keys+Values: accept multiple maps (vaargs) - Adding UniqKeys+UniqValues

[shivamrazorpay]

Resolved Issue : #452

Added :

• Accept Multiple maps as vaargs in Keys
• Accept Multiple maps as vaargs in Values
• Added UniqKeys which creates an array of unique map keys.
• Added UniqValues which creates an array of unique map values.
• Added Test for the same

[ccoVeille]

Coding is not only about thinking how to code the things you need to add for this feature, but to think about all the use cases your additional features could lead to.

[ccoVeille]

LGTM 👍 thanks

Let's wait for @samber feedback now

[samber]

According to your changes, you're doing 2 operations: list keys and dedup.

Can you argue on this behavior?

IMO, some developers would only need to list keys, eg: lo.CountValues(lo.Keys(a, b))

Others might want to list keys with deduplication, but it could be done with 2 methods:

• lo.Uniq(lo.Keys(a, b))
• or a faster implem: lo.UniqKeys(a, b)

Also, I'm thinking about a very similar helper: lo.Values. Would you dedup values? I don't think so.

So to push this work forward, I would suggest the following:

• vaarg on lo.Keys without dedup
• a new helper lo.UniqKeys
• vaarg on lo.Values without dedup
• a new helper lo.UniqValues

Any ideas on the matter?

[shivamrazorpay]

@samber seems like a fair callout (I was also thinking along the same lines) and also a good suggestion. I will make the 4 changes you have asked and will ask you to review them. Thank you for your suggestion.

[samber]

Sure. I'm also curious about other developer's opinions on this.

[ccoVeille]

I'm fine with the changes @samber suggest

I just want to make sure the code will be benchmarked against slices.Compact when using in-house dedup once you will get the keys or values

[shivamrazorpay]

@samber @ccoVeille added the required changes.
can you please review it? 🙏

[ccoVeille]

Maybe you could try this for the note

https://docs.github.com/en/get-started/writing-on-github/getting-started-with-writing-and-formatting-on-github/basic-writing-and-formatting-syntax#alerts

[ccoVeille]

Suggested change

	if _, exists := seen[k]; !exists {
	seen[k] = struct{}{}
	result = append(result, k)
	_, exists := seen[k]
	if exists {
	continue
	}
	seen[k] = struct{}{}
	result = append(result, k)

[shivamrazorpay]

@ccoVeille why is this change much better ?? Not able to understand. Is using continue a better approach ??

[ccoVeille]

It's the logic of "leave early", it's for readability and maintainability.

First, here, it's simpler to read that when existing we skip, than reading than when no existing we have to do something.

The leave early concept is there to avoid big if branch.

Also, imagine you have an new condition in this loop

With your current code, it would lead to put an if in the if

So the code would be something like this

for {
    if condition1 {
         something1
         if condition2 {
               something2 
         }
    }
}

Here is the same code when applying leave early

for {
    if !condition1 {
         continue
    }
    something1
    if !condition2 {
          continue
    }
    something2 
}

[shivamrazorpay]

waaaooo this seems a very good explanation. Thank you making it soo clear

[shivamrazorpay]

@samber can you review the PR ?? all the review comments are resolved.

[samber]

Please check the tests.

[shivamrazorpay]

@samber Sorry my bad. There I was asserting the 3rd test and sorted the 1st test slice.
Fixed it. I checked all the tests are now working.

[ccoVeille]

That's why I'm using a pattern I barely seen here.

func TestWhatever(t *testing.T) {


    t.Run("one test", func(t *testing.T) {
          // …
    })

    t.Run("another test", func(t *testing.T) {
          // …
    })

   t.Run("and so on", func(t *testing.T) {
          // …
    })
}

[d-enk]

may be more effective

seen := make(map[K]struct{})
	
for i := range in {
  for k := range in[i] {
	seen[k] = struct{}{}
  }
}

result := make([]K, 0, len(seen))

for k := range seen {
	result = append(result, k)
}

[d-enk]

similarly UniqValues

[samber]

with this version, the order is not guarantee

IMO, we would need a new helper (UniqKeysUnordered) ?