
Uber Go Style Guide - robfig
https://github.com/uber-go/guide/blob/master/style.md
======
sanxiyn
"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."

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!

~~~
skybrian
Yes, that's not good, but it seems like in practice this doesn't come up too
often in Go?

More typically you're building a new slice containing the results of a query
or other computation, and returning it without keeping a reference. Or instead
of exposing an internal map, you have a Get method.

~~~
morelisp
I have found this to be true of everything other than byte slices, where the
result is some of the worst bugs I've had the displeasure of tracking down.

Many Go libraries like to offer passing a byte slice to reuse as a
destination. Many Go libraries take byte slices or structs containing them as
arguments. A common result is "loop over some input, read it into the reusable
slice, pass the slice to the next step". It even sort of works - until the
next step does something asynchronous.

It doesn't help that:

\- Lots of things return a new slice "sometimes" \- _this_ append() result is
safe to pass on; this other one is not; this other one "it depends".

\- The `x[:]` idiom to turn an array into a slice looks exactly like a Python
copy, but actually shares the same backing store.

In the end we basically banned reusing slices as a result. (The rule I tell
new programmers is "if you pass the slice elsewhere in a loop, the slice needs
to be allocated within the loop.") Which is a shame, because the performance
gain from safe reuse is often significant. But until the type system blocks us
from changes, usually in the oblivious callee, which turn a safe use into an
unsafe one, it's simply too much effort to remember and review each case.

~~~
skybrian
Ouch. I guess this would be a reason to use strings more, since they're
immutable.

~~~
morelisp
Someone using a string when they really want a byte array/slice would fail
review on its face. Excepting external APIs forcing "strings" on us, we only
use the Go string type for UTF-8 code unit sequences (and this is a common
assumption in the Go world).

------
heroHACK17
I really like the horizontal 'Good/Bad' code comparisons in this guide.

I didn't realize how horizontal vs. vertical code comparison affects
readability; IMO horizontal is MUCH more readable.

Example: [https://github.com/uber-
go/guide/blob/master/style.md#defer-...](https://github.com/uber-
go/guide/blob/master/style.md#defer-to-clean-up)

~~~
klabb3
+1. It is quite an achievement by Go as a language and/or community to have
generally short lines of code where two columns fit in a regular website
width. I don't like everything about Go, but it's usually pretty easy on the
eyes for this reason.

~~~
apta
I don't think that the prevalence of single letter variable names can be
considered an achievement (which is how they get short lines to begin with).
It's quite a hindrance to readability. golang as a language does nothing to
support shorter lines.

~~~
morelisp
> golang as a language does nothing to support shorter lines.

It does a few things.

\- Most declarations, including method declarations, take place at the top
level. This is unlike most languages which declare methods one level indented
along with the object's fields.

\- Short type definitions (`type T1 T2`) are encouraged and often used for any
repeated non-scalar type. This shortens argument specifications and (if a good
name was chosen) improves readability.

\- Anonymous struct fields are shorter both to declare and use. In class-based
OO this is often the case for subclasses but not wrappers/facades. Go's
approach covers both.

\- Go linters push people to avoid `if x { ... return a } else { ... return b
}` in favor of `if x { ... return a } ... return b`. I actually hate this, but
it does keep indentation down.

\- The lack of exception handling means there is a constant "pressure" to push
errors back up manually, via early return if it's locally unhandled or to the
top of the current lexical scope of it is. The result is definitely verbose
and one of the easiest parts of Go to criticize - but the same pressure
results in short (but plentiful) error handling lines.

(I do agree the majority of savings comes from short variable names, though I
disagree that it affects readability much for the case of _variable names_ -
struct fields are another issue...)

~~~
apta
> \- Most declarations, including method declarations, take place at the top
> level. This is unlike most languages which declare methods one level
> indented along with the object's fields.

You can do the same in C++ or Kotlin, etc., but you don't find people saying
that those automatically make lines shorter.

> \- Short type definitions (`type T1 T2`) are encouraged and often used for
> any repeated non-scalar type. This shortens argument specifications and (if
> a good name was chosen) improves readability.

Also possible in C++ or Kotlin, among others.

> \- Anonymous struct fields are shorter both to declare and use. In class-
> based OO this is often the case for subclasses but not wrappers/facades.
> Go's approach covers both.

This can save lines of code (at the expense of not being able to easily figure
out what classes implement or embed other classes or interfaces), but does
nothing to shorten lines of code.

Same applies to error handling. Returning early can be useful in certain
situations, but cause readability issues in others. golang linters did a bad
job here as you point out.

~~~
morelisp
> You can do the same in C++ or Kotlin, etc., but you don't find people saying
> that those automatically make lines shorter.

I don't know what happens in the Kotlin community, but you absolutely find
people saying C++ is "less nested" than Java/Python/JavaScript/etc (and
claiming it as an advantage and disadvantage depending on their viewpoint).

I don't know what you're really arguing with here. Go gives you a construct
that makes lines shorter than most other OO languages. It's not "automatic" as
it's a feature of the language someone had to design - but regardless it is
shorter.

> [Short type definitions are] possible in C++ or Kotlin, among others.

Again I don't know Kotlin, but no, it's not possible in C++ (at least up
through the 11-and-a-bit I work with). The equivalent C++ would be "struct T1
: public T2", plus another line of "{}". And it's rare (for good-ish reasons)
to subclass specializations of the default containers rather than wrap them.

> [An anonymous structure] does nothing to shorten lines of code.

I don't see how this is a debatable point. Not having to specify the field
name in the definition is a shorter line than than having to specify it,
strictly so. Not having to specify the full path to such an embedded object to
call a method or access a field is nearly always shorter (it can be longer if
your struct name is longer than your field name would have been and you have
an ambiguous resolution - the former happens a lot, the second less, and both
in combination even less).

> Returning early can be useful in certain situations, but cause readability
> issues in others.

Regardless, it keeps the lines short.

You seem to want to argue "Go is ugly", which, fine. But the claim was "Go
doesn't do anything to help make shorter lines." It does quite a bit.

------
knorker
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.

Just one comment though:

> Embedding sync.Mutex

Never do this on an exported type. If you embed sync.Mutex that makes "Lock"
and "Unlock" part of _your_ exported interface. Now the caller of your API
doesn't know if _they_ are supposed to call Lock/Unlock, or if this is a pure
internal implementation detail.

your `type Stats` "isn't" a mutex. It _has_ a mutex.

~~~
meddlepal
> Never do this on an exported type. If you embed sync.Mutex that makes "Lock"
> and "Unlock" part of your exported interface. Now the caller of your API
> doesn't know if they are supposed to call Lock/Unlock, or if this is a pure
> internal implementation detail.

Isn't that exactly what they say?

> Embed for private types or types that need to implement the Mutex interface.

Personally, I would never embed a sync.Mutex though because even for an
internal type it is confusing.

~~~
knorker
Curious. Under "Returning Slices and Maps" they use it in their "Good"
example.

------
jrockway
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.)

A lot of people will do things like:

    
    
      func (s *Server) HandleFoo(ctx context.Context, in *input) status {
         ch <- workFor(in)
         return status.Accepted
      }
    

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:

    
    
      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())
         }
      }
    

Now we don't wait for the work to be accepted if the caller decides it doesn't
care about the job anymore.

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.

~~~
thezviad
the way to do is to pass context all the way through, until the other thing
you are waiting on uses up no resources.

so in your example it should be done like this: ch <\- workFor(ctx, in)

and the 'workFor' function should be the one that gets canceled with the
context.

Deep down at the very end, you would have select statement with '<-ctx.Done()'
and something else that doesn't need canceling (if it needs canceling it
should have taken context too).

Of course the world isn't perfect and not everything takes context right now
(even in standard library), so you do have to do some workarounds every once
in a while.

~~~
jrockway
This is a very trivial example so doesn't dive into all the complexity. There
is a lot of nuance here, like whether or not you really want to do an
operation in the background in the first place. Background work does means
that you lose the ability to apply backpressure to the calling system, and
that will cause a lot of problems under load. Even if you do want to do the
operation in the background, you still want to provide a (derived) context so
that you can link the background work to the initial request (at the very
least, for tracing purposes). That is omitted here for simplicity.

Where I was going with all of this was that the linked style guide mentions
cases where people create buffered channels presumably because their channel
writes start blocking. No buffer will make up for the case where the rate of
work requested is higher than the rate at which work can be completed. What
people really want in that case is the ability to get out of the channel write
and return an error to the downstream system; you don't want to buffer, you
want to cancel. There are many ways to accomplish the act of cancelling work,
but you do have to watch your channel writes explicitly.

~~~
fishywang
It does not need to be the background. You can check context cancellation
without any goroutines by:

    
    
        select {
        default:
        case <-ctx.Done():
            return ctx.Err()
        }
    

When there's no other channels to select with the context, just use that code
block between lengthy operations to check context cancellation and return
early.

(If you are curious about how many extra time is wasted, it's easy to write a
benchmark test for that. Last time I checked it was ~20ns when you are going
the default/not-cancelled route)

~~~
_caw
You may also simply write the following, which appears more elegant than the
select.

    
    
      if ctx.Err() != nil {
        //then the context has expired or been cancelled
      }

------
ra7
Is Go the primary language in Uber now? I see a lot of tools written in Go
coming out of Uber. What kind of services inside Uber is Go used for?

~~~
badrequest
Uber have well over 1,500 microservices written in Go, it is the primary
language backend services are written in at Uber.

~~~
OJFord
> Uber have well over 1,500 microservices written in Go

 _1500_?! I can't even imagine how micro a microservice could be such that
Uber could have so many.

edit: Having said that, the Monzo blog post also on the front page says it has
1100 microservices, so I suppose I've just not worked somewhere with 'actual'
microservices. What constitutes a 'service', a single procedure for RPC?

~~~
K0SM0S
I haven't worked with microservices directly but what I gather is:

\- build a UNIX philosophy, "efficient atomicity" so 1 microservice = 1
command

\- you then build logic with these like Lego bricks, that's your glue code =>
backend ties that together

On a typical base Linux install you have something between 250 and 1500
packages. Average Java repo has like, what, 80 files maybe? 120? Break it down
to functions / structs and you can see the 1500 being a reasonable estimate of
a modular architecture.

So you have one microservice to return a catalog, another to take a search
input, another to serialize this stuff, another to autofill something, another
to autofill some other thing, etc. Your app architecture schema is produced
by/in microservices.

That's the extreme approach to microservice architecture (the 1 command / 1
object / 1 function / 1 struct atomicity Lego-style), and in such a case most
microservices would typically look very much alike, with exactly the same
"general" parts (auth, db connect, etc) and just your business logic varying
in between.

With so small microservices, a team of 4 may be responsible for a good dozen
microservices, a team of 10 may handle a hundred... You quickly rise to high
hundreds in such scenarios.

Disclaimer: This is extensively parroted from the latest episode of "Go Time",
a fantastic podcast imho.

~~~
weberc2
This seems really excessive. You'd have to have really good tooling to build,
deploy, and debug (since log streams are oriented per-function, not per-
request), and I'm not sure how that could be performant, especially if you're
making O(N) (or worse) network calls per request. My understanding is that
microservices are typically 1-5 per team.

------
dev_dull
A style guide with benchmarks that can be read in a single cup of coffee. Very
nice and one of the reasons I enjoy Go!

------
sidlls
The section on exporting functions to check errors just exposes Go's weakness
in error checking. It's unnecessarily verbose.

~~~
badrequest
The way to do this properly got much better in the most recent Go release, the
linked documentation doesn't yet take that into account, it would seem.

[https://github.com/golang/go/wiki/ErrorValueFAQ](https://github.com/golang/go/wiki/ErrorValueFAQ)

~~~
sidlls
Error handling in go would be much better with a proper sum type, though. It's
very clunky as-is, even with the improvements outlined in that link

------
ArtWomb
No buffered channels, or if you do, you must provide a very strong rationale
;)

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!

~~~
dev_dull
I'd like to hear more about this rational. With a single writer, buffering
channels can help smooth out inputs when the p90 is much higher than the
average with channel writers but not readers. At least that's my impression.

~~~
jerf
Put in English, the type of an unbuffered channel is "a channel that always
blocks on writing until the value has been read". The type of a buffered
channel is "a channel that doesn't block when written to, until it is full in
which case it blocks on writing until _some other_ value has been read by some
other unrelated goroutine".

The former is a reasonable concurrency primitive. The latter superficially
seems similar, but is actually a much more complicated primitive, with the
corresponding code understanding problems and the increased likelihood of more
concurrency problems being hidden at low scale but coming out at scale (mostly
deadlocks), plus the fact it _seems_ similar is also problematic. The fact
that the Go type system does not allow distinguishing between the two is also
problematic.

It has some special-case purposes, but I also always scrutinize any channel
created with buffering to make sure it is one of those special cases.

Buffered channels have not impressed me with their ability to do any sort of
performance improvements. In almost all cases, if you've filled up your
readers, you really _want_ your writer to block. It provides cheap, effective
backpressure; arguably for software-at-scale it's the most useful aspect of
the channel primitive.

~~~
boyter
Backpressure to writers is the main reason to use buffer channels IMHO. I have
never seen any performance differences between buffered or unbuffered for
anything I have used.

The other reason to use them is controlling CPU. Spin up a pool of consumers
and control how many cores they use with the buffer.

------
tapirl
Copy Slices and Maps at Boundaries: [https://github.com/uber-
go/guide/blob/master/style.md#copy-s...](https://github.com/uber-
go/guide/blob/master/style.md#copy-slices-and-maps-at-boundaries)

The suggested good clone is not very efficient:
[https://github.com/go101/go101/wiki/How-to-efficiently-
clone...](https://github.com/go101/go101/wiki/How-to-efficiently-clone-a-
slice%3F)

In fact, I prefer the suggested bad one instead. We should let the caller to
determine whether or not a clone is needed.

\--------------------------

Start Enums at One: [https://github.com/uber-
go/guide/blob/master/style.md#start-...](https://github.com/uber-
go/guide/blob/master/style.md#start-enums-at-one) 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.)

\--------------------------

Local Variable Declarations: [https://github.com/uber-
go/guide/blob/master/style.md#local-...](https://github.com/uber-
go/guide/blob/master/style.md#local-variable-declarations)

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.

\--------------------------

Avoid Naked Parameters: [https://github.com/uber-
go/guide/blob/master/style.md#avoid-...](https://github.com/uber-
go/guide/blob/master/style.md#avoid-naked-parameters)

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.

If this must be done, I recommend to define and use some named bool constants
instead.

~~~
sanxiyn
Re: Start Enums at One. As explained, it is to avoid making the first enum
value the default when making it the default is not appropriate.

~~~
gen220
At my place of work, we pretty much always make the default (0) value
“Unspecified”.

The rationale is, it’s often quite useful to know that the originator hasn’t
specified a value, and to help new originators avoid unintended side-effects.

This style guide has the same effect, but it’s implicit; takes a few more
mental cycles to parse in exchange for less code. Personally, I think it’s
_probably_ worth the trade off to type it out once and save time thinking
later.

------
Groxx
I'll throw in one request for functional options _as written here_ : please
don't use closures to capture the data.

If you use closures, it's _nearly_ 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.

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.

(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)

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.

~~~
jonahx
are you saying you’re against the functional options approach?

can you provide an example of a testing difficulty you’ve had?

~~~
Groxx
I mean that this:

    
    
        func MyOption(arg int) Option {
          return myoption(func(opts type) { opts.thing = arg })
        }
    

will result in this:

    
    
        MyOption(5) == MyOption(5) // false
    

Which makes it a pain in 1) testing, since you can't see the options that were
passed, and 2) middleware, since you can't see the options that were passed,
to validate or extend them safely. And 3) debuggers or Printf since you can't
see the value in the closure until it's executed.

Instead do:

    
    
        type myOption int
        func MyOption(arg int) Option {
          return myOption(arg)
        }
    

since it has none of those problems.

Whether you use the "loop and opt.apply(arg)" pattern or not doesn't really
matter - that's a purely internal detail (personally I find it over-simplifies
things and causes problems). Just "avoid passing values through func
closures".

------
logicallee
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?)

~~~
dilap
nil when you’re not expecting it.

panic when there is a bug in your program; ie program not behaving as
expected, all bets off.

return err when expected conditions fail: bad data, service down, whatever —
handle gracefully.

------
aloknnikhil
> 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.

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.

------
palebluedot
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.

~~~
fishywang
Why having a 1000s buffered channel is useful in this case? If it's
unbuffered, you still get the backpressure blocking on writers as a feature,
since you have a worker queues of thousands of goroutines to read and handle
them.

~~~
palebluedot
That's a good point, at steady-state, I'd imagine unbuffered channels would
have the same throughput as a deeply buffered channels. The main advantage is
being able to spool up faster and smooth out throughput, but it could be for
most workloads that is not valuable enough. Perhaps I placed too much value on
that.

Your comment (and others) have convinced me to do some more empirical testing
and see how necessary buffered channels are for my goal.

------
marcrosoft
It’s nice that go barely needs a style guide because of go fmt.

------
fryh
I am not a Go programmer, but this seems like a really nice guide. Is there
anything similar for Python or C++?

~~~
magicbuzz
Yes, it's great. I know a few folks working on Go backends - I'm going to
point this out to them. And I think like you, it raises the question of 'Is
there a guide like this for what I'm working in?'

In my early React years, I had a lot of conversations about good vs bad as we
got used to working in a declarative component hierarchy rather than the old
imperative (tweak that DOM!) way. I still come across projects where people
write html + bootstrap translated into a render method rather than using the
power that is components.

------
abbot2
I find it amusing that the advice in the style guide gives a good example
contradicting another good example, and contains a subtle bug.

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.

(Handling that properly with a defer is a bit more tricky - something like
[https://play.golang.org/p/l1PeWM3Tisg](https://play.golang.org/p/l1PeWM3Tisg)).

Update: style guide was fixed after this report :) It was this if you wonder:
[https://github.com/uber-
go/guide/blob/a53ee0bef8c0b11b52340d...](https://github.com/uber-
go/guide/blob/a53ee0bef8c0b11b52340dbbc82e8be37f4a88d2/style.md)

------
thinkingkong
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.

~~~
mav3rick
Mostly by people following these and you bootstrap by some trusted people
getting +2 rights. As more people are trusted they get the rights.

------
nergal
Underscore in globals. Is that really go best practice?

~~~
tomohawk
Yes. Variables that start with lower case look like a locally defined variable
in a func. Adding the underbar makes it absolutely clear, and prevents
shadowing, or the need to work around shadowing with a slightly different
name.

------
codesushi42
Why is this better than Google's?

~~~
jrockway
This, to me, seems largely a superset of Google's style guide. There is some
specific guidance for working with channels, mutexes, and atomics. My
impression from my time at Google was that the Go team really expects you to
never use mutexes and atomics, preferring goroutines for all synchronization.
Sometimes that is impractical, though.

------
thallavajhula
Correct me if I'm wrong, but, isn't the primary purpose of `gofmt` to solve
having to deal with style guides like these in the community?

~~~
mattcdrake
This is referenced in the first paragraph of the style guide:

"Styles are the conventions that govern our code. The term style is a bit of a
misnomer, since these conventions cover far more than just source file
formatting—gofmt handles that for us."

------
jlokier
This recommendation surprised me:

    
    
      if err := ioutil.WriteFile(name, data, 0644); err != nil {
       return err
      }
    

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.

Of course it's not the assignment which is being tested in the Go code above.
But it might as well be.

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.

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.

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.

So I'm surprised Uber recommends this one.

~~~
PudgePacket
It helps reduce uninitialised variables, and if the condition is false, you
don't have an extra variable hanging around in the scope that could
potentially be misused by the developer later.

~~~
jlokier
You seem to have missed the point of the comment you're replying to, which
already acknowledges that scope-limiting is a good thing to do.

