Uber Go style should clarify ordering for type declarations and methods in files with multiple types

[andrewmains12]

I (and several other Uber engineers) are a bit confused by the https://github.com/uber-go/guide/blob/master/style.md#function-grouping-and-ordering section, specifically for the case where there are multiple types in one file.

Should it be:

// All type defs at top.

type something struct{ ... }
type somethingElse struct { ... }

func newSomething() *something {
    return &something{}
}

func (s *something) Cost() {
  return calcCost(s.weights)
}

func newSomethingElse() *somethingElse() {
  return &somethingElse{}
}
func (s *somethingElse) DoOtherThing() {

}

func calcCost(n []int) int {...}

or:

// Type defs near methods
type something struct{ ... }

func newSomething() *something {
    return &something{}
}

func (s *something) Cost() {
  return calcCost(s.weights)
}

type somethingElse struct { ... }


func newSomethingElse() *somethingElse() {
  return &somethingElse{}
}
func (s *somethingElse) DoOtherThing() {

}

func calcCost(n []int) int {...}

The style guide states:

Therefore, exported functions should appear first in a file, after struct, const, var definitions.

which makes me assume that "all type defs" at the top is correct, but it's not 100% clear from the example.

We should clarify this situation in the style guide (or, if the style guide has no opinion on it, state that as well).

[jkanywhere]

I would also appreciate clarification please.
Both of the above seem to follow

A newXYZ()/NewXYZ() may appear after the type is defined, but before the rest of the methods on the receiver.

I tend to defines type blocks at the top, then the constructors, then the methods.

// Type defs
type something struct{ ... }

type somethingElse struct { ... }

// constructors. Typically exported, but could be internal.
func newSomething() *something {
    return &something{}
}

func newSomethingElse() *somethingElse() {
  return &somethingElse{}
}

// methods alphabetical by receiver type, then method name.
func (s *something) Cost() {
  return calcCost(s.weights)
}

func (s *somethingElse) DoOtherThing() {

}

func calcCost(n []int) int {...}

I group /methods alphabetical by receiver type, then method name, which partially matches the style guide:

Functions should be sorted in rough call order.
Functions in a file should be grouped by receiver.

I dislike the idea of call order, because the order of calling methods can change and differ depending on the consumer. Alphabetical is more stable.

[jkanywhere]

Also related, the "Do not group declarations that are unrelated" good example shows

type Operation int

const (
  Add Operation = iota + 1
  Subtract
  Multiply
)

const EnvVar = "MY_ENV"

putting type before const in violation of my expectations, perhaps the guide itself?

I would expect to see const before type

const (
  Add Operation = iota + 1
  Subtract
  Multiply
)

const EnvVar = "MY_ENV"

type Operation int

For some reason, I expect the recommended file order is define all const, define global var, define all type.

But, I don't currently see that stated anywhere in the style guide.

[zmt]

Although https://github.com/uber-go/guide/blob/master/style.md#functional-options is somewhat a special case, the guidance there reads easily. My vote would be for guidance something like:

const sharedConsts ...
var sharedVars ...
type sharedTypesWithoutConstructorsIOrMethods...

type1...
type1Constructor...
type1Methods...
[type1SpecificUtilityFuncs // e.g. option pattern With funcs]...

type2...
type2Constructor...
type2Methods...
[type2SpecificUtilityFuncs // e.g. option pattern With funcs]...
...

exportedGeneralFuncs
unexportedGeneralFuncs

I would argue that enums are an exception as well so for enums, I see them as fit into the shared area near the top, but const & related type grouped together with the associated type defined first. They're more like a combination type so maybe below the shared const & shared vars, but above the shared types:

type enumType...
const (
	enumConst1 enumType = iota
	...
)

const 

Also for interface checks, I like them near the type they are checking, like so:

type1...
var _ CheckedInterface = (*type1)(nil) // presuming pointer in this example
type1Constructor...
type1Methods...
...
type type2 struct {...}
var _ CheckedInterface = type2{} // presuming non-pointer receivers in this example
type2Constructor...
type2Methods...
...

... but I digress ...

[TudorHulban]

Types should come first:
a. types are more abstract
b. you can define global variables in that file based on the types you define.

You can always group types in a new file so it does not get crowded. Accessing those types would be faster with good file naming.

[andrewmains12]

Just a vote for types next to method definitions: a struct and its methods are typically very tightly coupled to each other; it's much easier to understand the methods in the context of the state on the struct they manipulate. Type definitions away from methods means a lot more jumping around to understand a particular "class" (meaning here: struct with data + non trivial methods, vs just a struct with data).

Fwiw, this also matches other major languages much better; Python would have everything indented under the class header, Java would have everything in curly braces, etc.

That said I suspect the style guide authors already have an opinion on this, so we should probably just clarify (or alternatively, clarify that there is no recommended ordering).

[mway]

I'll clarify here before adding more detail to the guide, but at a high level, the guidance for ordering types in a given file is:

• for each defined type:
  • type definition
  • constructor(s), if applicable
  • initializer(s), if applicable
  • exported receiver methods
  • unexported recever methods

For example:

// example.go

type Foo struct {
  // ...
}

func NewFoo(/* ... */) Foo {
  // ...
}

func NewFooWithDuration(d time.Duration) Foo {
  // ...
}

func (f *Foo) ExportedFunc() {
  // ...
}

func (f *Foo) AnotherExportedFunc() {
  // ...
}

func (f *Foo) unexportedFunc() {
  // ...
}

type Bar struct {
  // ...
}

func NewBar(/* ... */) Bar {
  // ...
}

func (b *Bar) ExportedFunc() {
  // ...
}

func (b *Bar) AnotherExportedFunc() {
  // ...
}

func (b *Bar) unexportedFunc() {
  // ...
}

For enum-like consts, the guidance for ordering is:

• type definition (if applicable)
• enumerated values

type T uint8

const (
  InvalidT T = iota
  FooT
  BarT
)

which makes me assume that "all type defs" at the top is correct

This is not recommended. Receiver methods are strongly related to the types of their receivers, and they should be grouped together following the same general exported -> unexported ordering as free functions.

putting type before const in violation of my expectations, perhaps the guide itself?

This is a notable exception - we'll add clarification to the guide. Enum-like consts are effectively specializations of that enum type and should immediately follow the type definition, not precede it.

you can define global variables in that file based on the types you define.

All package-level const and var declarations should still be after imports, before general type definitions.

Happy to discuss further or field any additional questions here while we update the guide!

[jkanywhere]

I'll clarify here before adding more detail to the guide, but at a high level, the guidance for ordering types in a given file is:

• for each defined type:

  • type definition
  • constructor(s), if applicable
  • initializer(s), if applicable
  • exported receiver methods
  • unexported recever methods

I'm willing to follow that style when the guidance is clarified.

The example you provide is not alphabetized.

I request that we add an additional guidance that wthin each of the above bullets the items should be alphabetized. In other words: The first alphabetic type, alphabetized constructors, alphabetized initializers, alphabetized exported recevermethods, alphabetized unexported recever methods, next alphabetic type, etc.

"Call order" is brittle and will change over the life of the program. Alphabetic order is more stable.
If the function name changes, it will move within the file. If a function changes from exported to non-exported, it will move within the file, in this case likely the call order will change also.

// example.go

type Bar struct {
  // ...
}

func NewBar(/* ... */) Bar {
  // ...
}

func (b *Bar) AnotherExportedFunc() {
  // ...
}

func (b *Bar) ExportedFunc() {
  // ...
}

func (b *Bar) ThirdExportedFunc() {
  // ...
}

func (b *Bar) anotherUnexportedFunc() {
  // ...
}

func (b *Bar) unexportedFunc() {
  // ...
}

type Foo struct {
  // ...
}

func NewFoo(/* ... */) Foo {
  // ...
}

func NewFooWithDuration(d time.Duration) Foo {
  // ...
}

func (f *Foo) AnotherExportedFunc() {
  // ...
}

func (f *Foo) ExportedFunc() {
  // ...
}

func (f *Foo) unexportedFunc() {
  // ...
}

type Zot interface {
  // ...
}

Do we want to explicitly state to define all interfaces at the top? Or do we mix the interfaces alphabetically with the structs?

[mway]

The example you provide is not alphabetized.

I request that we add an additional guidance that wthin each of the above bullets the items should be alphabetized.

We don't want to micromanage folks that much, so we probably won't add additional ordering guidance aside from what needs to be grouped together. You're welcome to enforce that as a style guideline within your team/own codebases, though!

"Call order" is brittle and will change over the life of the program.

Yes, this is true - we will also be removing "call order" as explicit guidance and instead encourage teams to determine their own "readable order" within a given grouping. We've found that for stable codebases, call order can often be a cheap proxy for readability, but acknowledge that this isn't universally true (and so would rather leave it up to teams to determine what ordering is the most readable for their code - whether it's alphabetical, call order, or whatever else).

Do we want to explicitly state to define all interfaces at the top? Or do we mix the interfaces alphabetically with the structs?

We don't have official guidance for this (yet!), but in general, interfaces should be grouped and ordered the same as any other type definition: immediately preceding the types that implement them (where that makes sense), similar to how type definitions precede their constructors, et al. Ultimately, it will probably depend on context, so we'll probably only include guidance for common interface patterns. Agreed that it should be included with other ordering considerations, though.