mroth / weightedrand

:balance_scale: Fast weighted random selection for Go
MIT License
393 stars 50 forks source link

RFC: int vs uint for Choice.Weight #5

Closed mroth closed 4 years ago

mroth commented 4 years ago

Currently, Choice is defined such that:

type Choice struct {
    Item   interface{}
    Weight uint
}

My original intention here was from he "make invalid states unrepresentable" perspective -- and negative weights could cause errors, however #4 will introduce safety checks at creation time in NewChooser, making this less necessary.

However, it is potentially more idiomatic to utilize an int here, since it tends to be the numeric value type most commonly used by the Go community. The definition would then change slightly such that any Choice with Weight <= 0 would represent something that would never be selected. This could avoid potential typecasting (an ergonomics issue, not relevant to performance) when creating new Choices.

There are primary downside I can think of:

I'm currently mildly in favor of making this change now from an ergonomics perspective to hone in on the best API before stabilizing this library, but would love feedback from users. Otherwise it's probably not worth the disruption unless it's really better for users.

mroth commented 4 years ago

@0xERR0R do you have an opinion on this, by chance?

0xERR0R commented 4 years ago

@mroth thanks for asking for my opinion ;) Well, from my point of view, a negative weight doesn't make sense. Of course, you can handle negative values as 0, but there is a chance, that somebody can misunderstand your API. I would prefer clear interface: weight is always positive and therefor it is absolutely ok to use an uint. I'm aware of this "ideomatic" stuff in golang, but just IMHO it is not always the best way to solve problems.

mroth commented 4 years ago

Going to stick with uint for now!