Added Bin Sort [Go] #537
Added Bin Sort [Go] #537
Conversation
|
Hi! Thanks for showing interest in contributing to this repository. Please make sure that you have ticked the points mentioned in PR description correctly. Thanks again! |
|
The build failed due to Travis CI using older version of Go (<1.8). I'll update the code to use sort.Sort instead of sort.Slice. |
|
|
||
| import ( | ||
| "fmt" | ||
| "math/rand" |
There was a problem hiding this comment.
exported type Bucket should have comment or be unexported
Origin: GoLintBear, Section: golint.
bin_sort/bin_sort.go
Outdated
| // Methods required to allow sorting bucket's content | ||
| func (b Bucket) Len() int { return len(b) } | ||
| func (b Bucket) Swap(i, j int) { b[i], b[j] = b[j], b[i] } | ||
| func (b Bucket) Less(i, j int) bool { return b[i] < b[j] } |
There was a problem hiding this comment.
Please break these definitions in multiple lines.
There was a problem hiding this comment.
Ok. Do I need to expand to multiple lines or these single-line definitions are good to go?
There was a problem hiding this comment.
Multiple lines would be good guess.
bin_sort/bin_sort.go
Outdated
| // Methods required to allow sorting bucket's content | ||
| func (b Bucket) Len() int { return len(b) } | ||
| func (b Bucket) Swap(i, j int) { b[i], b[j] = b[j], b[i] } | ||
| func (b Bucket) Less(i, j int) bool { return b[i] < b[j] } |
There was a problem hiding this comment.
Should it be less instead of Less? Same for Swap.
There was a problem hiding this comment.
sort.Sort() requires these methods to be public (starting with capital letter)
bin_sort/bin_sort.go
Outdated
| type Bucket []float32 | ||
|
|
||
| // Methods required to allow sorting bucket's content | ||
| func (b Bucket) Len() int { return len(b) } |
There was a problem hiding this comment.
Better to use len(b) directly.
There was a problem hiding this comment.
Yes, but we have to implement Len method, as it's required for sort.Sort().
bin_sort/bin_sort.go
Outdated
| ) | ||
|
|
||
| // Bucket is used to store elements in BucketSort | ||
| type Bucket []float32 |
There was a problem hiding this comment.
Please avoid using global variables.
There was a problem hiding this comment.
It's not a global variable, it's a type alias. I made for the sake of easier understanding. I'll improve the comment to explain that.
There was a problem hiding this comment.
Okay! Yes, I missed it. A comment would be good :)
|
I'll update the code later today. |
|
@sergobot: Sure! Will be waiting :) |
|
Weird. I just have committed changes to my fork but they don't show up here 😕 |
bin_sort/bin_sort.go
Outdated
| sorted = append(sorted, buckets[i][j]) | ||
| } | ||
| } | ||
|
|
There was a problem hiding this comment.
Please remove this blank line.
bin_sort/bin_sort.go
Outdated
| for i := range arr { | ||
| arr[i] = rand.Float32() | ||
| } | ||
|
|
There was a problem hiding this comment.
Please remove this blank line.
|
@sergobot: Thanks for the contribution :) |
|
Yay! |
Fixes #526
By submitting this pull request I confirm I've read and complied with the below declarations.
Added {Algorithm/DS name} [{Language}], notUpdate README.mdorAdded new code.