"Copy Slices and Maps at Boundaries. Slices and maps contain pointers to the underlying data so be wary of scenarios when they need to be copied. Keep in mind that users can modify a map or slice you received as an argument if you store a reference to it. Similarly, be wary of user modifications to maps or slices exposing internal state."<p>This could be used as an ad for Rust borrow checker, verbatim. You can't modify a map or slice you passed as an argument if a reference to it is stored!
I really like the horizontal 'Good/Bad' code comparisons in this guide.<p>I didn't realize how horizontal vs. vertical code comparison affects readability; IMO horizontal is MUCH more readable.<p>Example: <a href="https://github.com/uber-go/guide/blob/master/style.md#defer-to-clean-up" rel="nofollow">https://github.com/uber-go/guide/blob/master/style.md#defer-...</a>
Pretty good. Some opinions that fall under "be consistent within our code base" (which is why Uber should have a style guide at all), so all good.<p>Just one comment though:<p>> Embedding sync.Mutex<p>Never do this on an exported type. If you embed sync.Mutex that makes "Lock" and "Unlock" part of <i>your</i> exported interface. Now the caller of your API doesn't know if <i>they</i> are supposed to call Lock/Unlock, or if this is a pure internal implementation detail.<p>your `type Stats` "isn't" a mutex. It <i>has</i> a mutex.
Something that I don't see style guides addressing, but think they should, is how to cancel work when the caller no longer cares about the answer. (Consider an aborted RPC, or someone pressing the "stop" button in their browser.)<p>A lot of people will do things like:<p><pre><code> func (s *Server) HandleFoo(ctx context.Context, in *input) status {
ch <- workFor(in)
return status.Accepted
}
</code></pre>
This will block on the channel write even if the context becomes cancelled. Instead, you really need to be explicit about what you want to block:<p><pre><code> func (s *Server) HandleFoo(ctx context.Context, in *input) status {
select {
case ch <- workFor(in):
return status.Accepted
case <- ctx.Done():
return status.Error("handle foo: wait for work to be accepted: %w", ctx.Err())
}
}
</code></pre>
Now we don't wait for the work to be accepted if the caller decides it doesn't care about the job anymore.<p>My impression from digging around the open source world is that nobody except me cares about this. So maybe I'm just crazy. But I really like not leaking goroutines on long-running programs, making Control-C behave gracefully, etc.
No buffered channels, or if you do, you must provide a very strong rationale ;)<p>I still use them quite a bit. Particularly synchronizing large rule sets as bit arrays. Of course you can use sync.Wait instead. But make(chan bool, N) semantics are just more convenient. It's stealth synchronization as a by-product. And hence the warnings about determinism!
Copy Slices and Maps at Boundaries: <a href="https://github.com/uber-go/guide/blob/master/style.md#copy-slices-and-maps-at-boundaries" rel="nofollow">https://github.com/uber-go/guide/blob/master/style.md#copy-s...</a><p>The suggested good clone is not very efficient: <a href="https://github.com/go101/go101/wiki/How-to-efficiently-clone-a-slice%3F" rel="nofollow">https://github.com/go101/go101/wiki/How-to-efficiently-clone...</a><p>In fact, I prefer the suggested bad one instead. We should let the caller to determine whether or not a clone is needed.<p>--------------------------<p>Start Enums at One: <a href="https://github.com/uber-go/guide/blob/master/style.md#start-enums-at-one" rel="nofollow">https://github.com/uber-go/guide/blob/master/style.md#start-...</a> Any reason here? (Edit: I just missed the reason. However, I think there should be a default value for most enums and most of the default values should zero.)<p>--------------------------<p>Local Variable Declarations: <a href="https://github.com/uber-go/guide/blob/master/style.md#local-variable-declarations" rel="nofollow">https://github.com/uber-go/guide/blob/master/style.md#local-...</a><p>Personally, I prefer "var x = ..." and think short variables should be used as limited as possible. I remember that Go team has a not-very-concrete plan to depreciate short variable declarations.<p>--------------------------<p>Avoid Naked Parameters: <a href="https://github.com/uber-go/guide/blob/master/style.md#avoid-naked-parameters" rel="nofollow">https://github.com/uber-go/guide/blob/master/style.md#avoid-...</a><p>This suggestion has its own drawback. When a parameter name changed, all the places using it must be modified to keep consistent. This is one reason why Go team rejected named arguments.<p>If this must be done, I recommend to define and use some named bool constants instead.
I'll throw in one request for functional options <i>as written here</i>: please don't use closures to capture the data.<p>If you use closures, it's <i>nearly</i> impossible to compare the results for equality in tests or code that might care to validate / deduplicate / whatever those args. You can achieve it with a moderate amount of heavily-implementation-dependent reflection (get the func type, construct arg(s), call func, check result), but that's about it.<p>Please just use values as the backing type. `type thing int` is utterly trivial to compare if needed, by comparison - just use `==`. Any code anywhere can construct the same argument in the same way and `==` to see if what they are checking is part of the args, and you're still not binding yourself to a specific implementation-type that'll later risk a compile-time failure.<p>(if users are casting to the private `int` type to check stuff, yea, it breaks - but the reflection-to-read-closure approach has that same problem, you can't stop it)<p>Also, in my experience, the "loop and apply" pattern tends to fall apart rather quickly, and you start wanting more complicated interactions between args / validation that you didn't pass both "include deleted" and "exclude deleted" and the second just silently clobbered the first. With values you can pretty easily loop and switch on the type and do whatever you need.
Not an expert, but could someone explain why it says: "Panic/recover is not an error handling strategy. A program must panic only when something irrecoverable happens such as a nil dereference." Why is that any more irrecoverable than anything else? (You can check if it's nil before referencing it, right?)
> Use go.uber.org/atomic
Atomic operations with the sync/atomic package operate on the raw types (int32, int64, etc.) so it is easy to forget to use the atomic operation to read or modify the variables.
go.uber.org/atomic adds type safety to these operations by hiding the underlying type.<p>I don't agree with this guideline. When you anyway have to remember to invoke an intrinsic function on the variable (unlike, say, operator overloading), I wonder why it's necessary to include yet another dependency that's just a wrapper for sync/atomic all because the developer doesn't pay attention to the right use of atomics.
The preference of channel size being unbuffered or just 1 is interesting. That seems like something specific to a problem domain; for instance, in projects I am working on now, having a large buffered channel (1000s deep) is useful for worker queues of thousands of goroutines, that all read from a task feeder channel. This type of queuing seems go-idiomatic, and negates the need for additional synchronization. In this case, the backpressure blocking on writers is a feature.
I find it amusing that the advice in the style guide gives a good example contradicting another good example, and contains a subtle bug.<p>In the "Reduce Scope of Variables", second good example leaks an open file when WriteString fails, because it doesn't follow the own advice of "Defer to Clean Up" if you are curious.<p>(Handling that properly with a defer is a bit more tricky - something like <a href="https://play.golang.org/p/l1PeWM3Tisg" rel="nofollow">https://play.golang.org/p/l1PeWM3Tisg</a>).<p>Update: style guide was fixed after this report :) It was this if you wonder: <a href="https://github.com/uber-go/guide/blob/a53ee0bef8c0b11b52340dbbc82e8be37f4a88d2/style.md" rel="nofollow">https://github.com/uber-go/guide/blob/a53ee0bef8c0b11b52340d...</a>
How would you enforce any of these? Are these guides or reasons to not accept PRs? Seems like it would be worth encoding in a tool if this is for teams.
This recommendation surprised me:<p><pre><code> if err := ioutil.WriteFile(name, data, 0644); err != nil {
return err
}
</code></pre>
I've often seen guides for many languages that say don't use an assignment as an "if" condition. It may be a typo, and so is a source of errors, or it hides errors. Many compilers warn about it, and some people will consider it poor enough to refactor it out.<p>Of course it's not the assignment which is being tested in the Go code above. But it might as well be.<p>In Swift, Rust, Clojure and Perl, "if let" is common. ("if-let" in Clojure, "if my" in Perl). It's useful, and conforms well to the idea of limiting scope of the tested variable. I use it all the time.<p>So scope limiting in "if" is a good idea. But in those languages, they have the benefit of a clear syntax with a keyword, and it's a single assignment+test operator, very unlikely to be an accident due to a typo, or to hide an accident.<p>In contrast, I think the Go snippet looks error prone because the actual condition is all the way off the right. The assignment is obvious and the intention to test it can be presumed when skimming the code. But because the test is way off the right, at a horizontal position which will vary in different code, and at the end of a long line with a compound statement, I think it will be easy when skimming code to fail to notice if the condition is wrong due to a typo.<p>So I'm surprised Uber recommends this one.