Open jpgoldberg opened 6 years ago
I didn't even notice that I spelled choose wrong. I have some time coming up later this week and wanted to spend some time on this library, so i will take a deeper look at this (and other PRs) then.
for why floats are used. . . I can't remember. I wrote this when I was first learning Go and it has many many issues that I need to go back and fix.
So other than the benchmarks it looks fine
func Benchmark_NChoseK(b *testing.B) {
for n := 0; n < b.N; n++ {
NChoseK(100,2)
}
}
func Benchmark_NChoseKOld(b *testing.B) {
for n := 0; n < b.N; n++ {
NChoseKOld(100,2)
}
}
Benchmark_NChoseK-4 5000000 368 ns/op
Benchmark_NChoseKOld-4 300000000 4.42 ns/op
To see how this would impact the who algorithm i created these benchmarks.
func Benchmark_1(b *testing.B) {
for n := 0; n < b.N; n++ {
GoPasswordStrength("zxcvbn", nil)
}
}
func Benchmark_2(b *testing.B) {
for n := 0; n < b.N; n++ {
GoPasswordStrength("Tr0ub4dour&3", nil)
}
}
func Benchmark_3(b *testing.B) {
for n := 0; n < b.N; n++ {
GoPasswordStrength("neverforget13/3/1997", nil)
}
}
func Benchmark_4(b *testing.B) {
for n := 0; n < b.N; n++ {
GoPasswordStrength("briansmith4mayor", nil)
}
}
func Benchmark_5(b *testing.B) {
for n := 0; n < b.N; n++ {
GoPasswordStrength("Ba9ZyWABu99[BK#6MBgbH88Tofv)vs$", nil)
}
}
func Benchmark_6(b *testing.B) {
for n := 0; n < b.N; n++ {
GoPasswordStrength("rWibMFACxAUGZmxhVncy", nil)
}
}
With Old
Benchmark_1-4 2000 657936 ns/op
Benchmark_2-4 2000 711202 ns/op
Benchmark_3-4 2000 941439 ns/op
Benchmark_4-4 10000 142220 ns/op
Benchmark_5-4 2000 1073096 ns/op
Benchmark_6-4 10000 147925 ns/op
With New NChoseK
Benchmark_1-4 2000 659010 ns/op
Benchmark_2-4 2000 724277 ns/op
Benchmark_3-4 2000 965038 ns/op
Benchmark_4-4 10000 147744 ns/op
Benchmark_5-4 1000 1107561 ns/op
Benchmark_6-4 10000 152362 ns/op
@jpgoldberg What do you think?
A comment in the original said,
The choose function is also known as the binomial coefficient, and this exists in math/big for Ints.
What
This PR replaces the contents of
NChoseK()
with a call to math/big'sBinomial()
. It does some conversions so that no changes need to be made in how NChoseK is called.Why?
Why make this change?
What this doesn't do
Possible objections
Although I do acknowledge those as legitimate objections, I wouldn't be submitting this PR if I didn't think the benefits outweigh them.