Decode shouldn't panic
Closed this issue · 3 comments
Hashed ids are often used as part of urls. They are outside of program's control (e.g. a user on the website might manually modify url).
If such modified value is passed to Decode(), it'll panic, which is against Go conventions, see http://blog.golang.org/defer-panic-and-recover:
The convention in the Go libraries is that even when a package uses panic internally, its external API still presents explicit error return values.
It would be better if it was Decode(hash string) ([]int, error).
This would require breaking the API, which might not be a good idea, so an alternative would be to provide additional DecodeSafe (not a great name) or IsValid(hash string) bool so that the string can be checked for correctness before calling Decode().
panic()-ing is not just annoyance but might be a serious issue. If there's a sync.Mutex.Lock() that is unlocked without using defer anywhere in the request handling go-routine, panic will cause permanent deadlock of the whole web app. This happened in my app.
Very good point, will take a look when I get home.