Replace generateRandomString with id.Generate. #30
Conversation
The new id.Generate function uses crypto/rand instead of math/rand. This is more appropriate for generating random IDs.
| return n.Text(36), nil | ||
| } | ||
|
|
||
| var max = new(big.Int).SetUint64(math.MaxUint64) |
There was a problem hiding this comment.
I find it weird that this constant is defined at the bottom instead of at the top. Is this a Go convention?
There was a problem hiding this comment.
It is indeed a pattern that happens in many Go projects, and it's counter to some other languages such as C/C++ where dependencies tend to be defined/declared first before things that use them.
In Go, package level declaration order largely doesn't matter, the compiler figures out the order based on which identifiers use/mention others (see https://golang.org/ref/spec#Package_initialization), so you can arrange them in whatever way is most readable.
It turns out that the best readability of code can be achieved, more often than not, by placing the highest level operations first, and the lower level helpers second. The reason it works well is because humans tend to read from top to bottom, and reading the higher level functions gives you context for the lower level helpers. Once you see where the helpers are used, you can then understand why they're needed and what they do, when you see their implementation details later.
I was indeed making this declaration order intentionally and thoughtfully; it wasn't random.
There was a problem hiding this comment.
That said, in this case, max is a very simple variable (effectively a constant) rather than a complicated low-level implementation, so putting it at the top would've been equally acceptable.
| ) | ||
|
|
||
| // Generate a random ID like "1njfizqgeukrq", "3l44ze7pf47fd", "35dgd4n5ryup", etc. | ||
| func Generate() (id string, err error) { |
There was a problem hiding this comment.
I would prefer a function that takes the output string length and a collection of possible characters to put into the output string, but this will do.
There was a problem hiding this comment.
Package id is created specifically for InstantShare needs and not meant to be a general purpose one. At least at this time, since InstantShare is the only intended user. We could even make it an internal package to enforce no one else can import it, but I don't think it's needed because no one will anyway. Making packages internal has a cost of making the import path slightly more verbose.
So, that's why I chose to make the exported API of the package as simple as possible. If InstantShare needs to change, say, length of IDs or characters used, we can modify this package directly.
If InstantShare needs to to support variable ID length, etc., then we can make it general.
The principle we follow here is to avoid prematurely creating unnecessary abstractions. Instead, we write the simplest code that is actually needed today. Such code is easier to understand, to use, to modify, to maintain. This is pretty commonly done in Go projects.
The new
id.Generatefunction usescrypto/randinstead ofmath/rand.This is more appropriate for generating random IDs.