Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

3033 fix GetRandom* #1

Merged
merged 3 commits into from
Nov 22, 2018
Merged

3033 fix GetRandom* #1

merged 3 commits into from
Nov 22, 2018

Conversation

akaspin
Copy link

@akaspin akaspin commented Nov 21, 2018

@akaspin akaspin requested a review from dynamix November 21, 2018 14:03
@akaspin akaspin added the bug Something isn't working label Nov 21, 2018
Copy link

@dynamix dynamix left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

having a single function and passing the slice makes sense 👍 but pickRandom seems to complicated. Check my proposal.

random.go Outdated
return v
}

for i := 0; i < length; i++ {
Copy link

@dynamix dynamix Nov 21, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This seems too complicated. Why not?

for l,r := from-1,from+1; l>=0||r<len(slice);l, r = l-1, r+1 {
	if r >= 0 && r < len(slice) && len(slice[r]) > 0 {
		return slice[r]
	}
	if l >= 0 && l < len(slice) && len(slice[l]) > 0 {
		return slice[r]
	}
}  

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

  1. Your approach was implemented first. BUT corrected behaviour always tries to pick random from l or r. This give more “randomness”.

hmmm. sec.

random.go Outdated
}

for l, r := from-1, from+1; l >= 0 || r < len(slice); l, r = l-1, r+1 {
if r < len(slice) && len(slice[r]) > 0 {
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Unlikely but what happens if r or l overflow?

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@akaspin an additional check won't hurt

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

from can not be less or greater than length
l can not be greater than from-1
r can not be less than from+1

I'll make commit with tests and small improvements.

@dynamix dynamix added the Approved This PR was approved. label Nov 21, 2018
@akaspin akaspin merged commit 6eb50e9 into master Nov 22, 2018
@akaspin akaspin deleted the 3033-fix-random branch November 22, 2018 09:09
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Approved This PR was approved. bug Something isn't working
Development

Successfully merging this pull request may close these issues.

2 participants