3 ms·

First off, I should clarify that I'd actually love to see Go add support for proper enums, as I've certainly found it lacking. My biggest issues would be maint

by sjroot 6y ago

First off, I should clarify that I'd actually love to see Go add support for proper enums, as I've certainly found it lacking. My biggest issues would be maintainability and, as you mentioned, using global maps. If your enum itself is not a string value, but you NEED a string representation, then it should just implement `Stringer`. As far as maintainability: - The naming convention used here is not consistent with how enums are named in Go. For a good example, see the HTTP package, specifically request methods and response status codes [1]. Really, it isn't even an appropriate naming convention for constant values in any language, with `UPPERCASE_UPPERCASE_minor_note`, it looks a bit silly and took longer for me to grok. - What if I want to add a new enum variant? I seriously have to add it to three different places? If I start touching this map in places outside the original source file, things will get really ugly really fast. - A bit nit-picky, but: why use an `int32` here? This file, in my opinion, is over-engineered. Go code should be simple; many people learning Go for the first time hesitate to work that way, unfortunately. Other common issues I see with people learning Go for the first time: - Overuse of concurrency. People like to use the `go` statement wherever possible, and create overly-complex APIs with channels. Unless you have multiple events that need to be done in parallel, yet simultaneously needing to communicate between them, you should not use them in your package's public API. Let your package consumers choose when to make that call. - Package structure, particularly in modules. People like `src` directories, but Go doesn't work that way. Your top priority as someone learning Go should be to dig through Go's standard library itself and see how it is organized. [1] https://golang.org/pkg/net/http/#pkg-constants https://golang.org/pkg/net/http/#pkg-constants

ed25519FUUU 6y ago
> Overuse of concurrency. People like to use the `go` statement wherever possible, and create overly-complex APIs with channels. Unless you have multiple events that need to be done in parallel, yet simultaneously needing to communicate between them, you should not use them in your package's public API. Let your package consumers choose when to make that call. Agree. As much as possible, always leave it up to your caller on whether or not you want the blocking calls to run concurrently. Over-use of `go` as well as unnecessarily buffered channels are two really common anti-patterns with new Go developers. And don't get me wrong, they're not breaking or absolutely terrible design choices, it's just that they're choices you typically wouldn't make after spending quality time writing apps with the language.
- sjroot 6y ago
  Agreed! In my situation, lots of developers were porting Java APIs to go. They essentially had to be rewritten twice: 1st way: the way our Java devs thought Go was “supposed” to be written 2nd way: how Go should probably actually be written By embracing the simplicity and Go’s opinionatedness the first time around, you’ll save yourself a lot of refactoring headache later.
heleninboodler 6y ago
> If your enum itself is not a string value, but you NEED a string representation, then it should just implement `Stringer`. I don't understand. Isn't this exactly what they did? The only actual problem I see with this enum implementation is that the module-level value->string map is module public, which should definitely be fixed. Other than that, this looks like a bog-standard enum implementation with slightly odd internal naming conventions. > I seriously have to add it to three different places? Well, come on, now. The idiomatic example has two places to edit as well, and the third place here is for functionality the idiomatic example doesn't have (mapping back from string to value). You could always generate this map at runtime in an init() function but I suspect some would complain about that as well.

by sjroot 6y ago

ed25519FUUU 6y ago

> Overuse of concurrency. People like to use the `go` statement wherever possible, and create overly-complex APIs with channels. Unless you have multiple events that need to be done in parallel, yet simultaneously needing to communicate between them, you should not use them in your package's public API. Let your package consumers choose when to make that call. Agree. As much as possible, always leave it up to your caller on whether or not you want the blocking calls to run concurrently. Over-use of `go` as well as unnecessarily buffered channels are two really common anti-patterns with new Go developers. And don't get me wrong, they're not breaking or absolutely terrible design choices, it's just that they're choices you typically wouldn't make after spending quality time writing apps with the language.

sjroot 6y ago
Agreed! In my situation, lots of developers were porting Java APIs to go. They essentially had to be rewritten twice: 1st way: the way our Java devs thought Go was “supposed” to be written 2nd way: how Go should probably actually be written By embracing the simplicity and Go’s opinionatedness the first time around, you’ll save yourself a lot of refactoring headache later.

heleninboodler 6y ago

> If your enum itself is not a string value, but you NEED a string representation, then it should just implement `Stringer`. I don't understand. Isn't this exactly what they did? The only actual problem I see with this enum implementation is that the module-level value->string map is module public, which should definitely be fixed. Other than that, this looks like a bog-standard enum implementation with slightly odd internal naming conventions. > I seriously have to add it to three different places? Well, come on, now. The idiomatic example has two places to edit as well, and the third place here is for functionality the idiomatic example doesn't have (mapping back from string to value). You could always generate this map at runtime in an init() function but I suspect some would complain about that as well.