Skip to content

Dev: Coding Conventions

Jump to bottom
Armel Soro edited this page Jun 14, 2022 · 10 revisions

Goals

Guidelines exist to keep a codebase manageable in the long run, while still allowing developers to use programming languages features productively.

Even if Go is a more opinionated language than many other languages when it comes to coding style, there are still some areas not covered and that we would need to agree upon.

Rather than reinventing the wheel, the idea here is to adopt battle-tested conventions out there and enhance them as needed to better meet our own needs for odo.

The guidelines below are mostly extracted from the following popular conventions and recommendations:

Guidelines

Validation

Guideline Identifier: Validation

All code should be error-free when run through our validation tools, via the following command: make validate.

This is already enforced in CI, but think about running this frequently, as a personal habit.

Separate layers

See the following issues for more context:

The codebase strives to be organized as follows:

  • low-level layer that interacts with the cluster
  • business layer that defines abstractions of the domain. Think of this as a library that could potentially get consumed by other tools, like any other application
  • CLI layer that defines the UI

layers

Output in business layer

Guideline Identifier: Layers_OutputInBusinessLayer

Business layer should remain generic because it aims at being usable by other tools, like for JSON output. Packages in this layer should ideally manipulate objects only and return error objects as needed. It would then be up to callers to decide what to do with the returned objects.

As such, it is recommended not to print anything in this layer, as it may mess with callers that wish to control output, like for JSON output. Anything that needs to get logged in this layer should ideally go to stderr or a log file. Better, instead of outputting to stderr or stdout, methods in business layers should accept an io.Writer object so that their code is more testable.

To decouple the business layer from the UI representation, it is recommended to leverage the Callback/Handler pattern (more on that in the Patterns section). For example, by making the CLI pass a callback function (say CLICallback, responsible for printing to the console) to the business layer. The business layer would then call CLICallback when needed, and that function CLICallback would print the appropriate messages.

Do not import CLI layer packages in business layer

Guideline Identifier: Layers_CLIImportInBusiness

Business layer should not depend on packages in the CLI layer.

Verify Interface Compliance

Guideline Identifier: InterfaceCompliance

Verify interface compliance at compile time where appropriate. This includes:

  • Exported types that are required to implement specific interfaces as part of their API contract
  • Exported or internal types that are part of a collection of types implementing the same interface
  • Other cases where violating an interface would break users
Not Recommended Good 
type Handler struct {
  // ...
}

func (h *Handler) ServeHTTP(
    w http.ResponseWriter, r *http.Request) {
    // ...
}
 
type Handler struct {
  // ...
}

var _ http.Handler = (*Handler)(nil)

func (h *Handler) ServeHTTP(
    w http.ResponseWriter, r *http.Request) {
    // ...
}

The statement var _ http.Handler = (*Handler)(nil) will fail to compile if *Handler ever stops matching the http.Handler interface.

The right-hand side of the assignment should be the zero value of the asserted type. This is nil for pointer types (like *Handler), slices, and maps, and an empty struct for struct types.

type LogHandler struct {
  h   http.Handler
  log *zap.Logger
}

var _ http.Handler = LogHandler{}

func (h LogHandler) ServeHTTP(w http.ResponseWriter, r *http.Request) {
  // ...
}

Zero-value Mutexes are valid

Guideline Identifier: ZeroValueMutexesAreValid

The zero-value of sync.Mutex and sync.RWMutex is valid, so you almost never need a pointer to a mutex.

Not Recommended Good 
mu := new(sync.Mutex)
mu.Lock()
 
var mu sync.Mutex
mu.Lock()

If you use a struct by pointer, then the mutex should be a non-pointer field on it. Do not embed the mutex on the struct, even if the struct is not exported.

Not Recommended Good 
type SMap struct {
  sync.Mutex

  data map[string]string
}

func NewSMap() *SMap {
  return &SMap{
    data: make(map[string]string),
  }
}

func (m *SMap) Get(k string) string {
  m.Lock()
  defer m.Unlock()

  return m.data[k]
}
 
type SMap struct {
  mu sync.Mutex

  data map[string]string
}

func NewSMap() *SMap {
  return &SMap{
    data: make(map[string]string),
  }
}

func (m *SMap) Get(k string) string {
  m.mu.Lock()
  defer m.mu.Unlock()

  return m.data[k]
}

The Mutex field, and the Lock and Unlock methods are unintentionally part of the exported API of SMap.

The mutex and its methods are implementation details of SMap hidden from its callers.

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.

Receiving Slices and Maps

Guideline Identifier: CopySlicesAndMapsAtBoundaries_ReceivingSlicesAndMaps

Keep in mind that users can modify a map or slice you received as an argument if you store a reference to it.

Not Recommended Good 
func (d *Driver) SetTrips(trips []Trip) {
  d.trips = trips
}

trips := ...
d1.SetTrips(trips)

// Did you mean to modify d1.trips?
trips[0] = ...
 
func (d *Driver) SetTrips(trips []Trip) {
  d.trips = make([]Trip, len(trips))
  copy(d.trips, trips)
}

trips := ...
d1.SetTrips(trips)

// We can now modify trips[0]
// without affecting d1.trips.
trips[0] = ...

Returning Slices and Maps

Guideline Identifier: CopySlicesAndMapsAtBoundaries_ReturningSlicesAndMaps

Similarly, be wary of user modifications to maps or slices exposing internal state.

Your exported API should refrain from returning internal state that otherwise has accessors, e.g. when it is hidden behind a mutex.

Not Recommended Good 
type Stats struct {
  mu sync.Mutex
  counters map[string]int
}

// Snapshot returns the current stats.
func (s *Stats) Snapshot() map[string]int {
  s.mu.Lock()
  defer s.mu.Unlock()

  return s.counters
}

// snapshot is no longer protected by the mutex,
// so any access to the snapshot is subject to
// data races.
snapshot := stats.Snapshot()
 
type Stats struct {
  mu sync.Mutex
  counters map[string]int
}

func (s *Stats) Snapshot() map[string]int {
  s.mu.Lock()
  defer s.mu.Unlock()

  result := make(map[string]int, len(s.counters))
  for k, v := range s.counters {
    result[k] = v
  }
  return result
}

// Snapshot is now a copy.
snapshot := stats.Snapshot()

Defer to Clean Up

Guideline Identifier: DeferToCleanUp

Use defer to clean up resources such as files and locks.

Not Recommended Good 
p.Lock()
if p.count < 10 {
  p.Unlock()
  return p.count
}

p.count++
newCount := p.count
p.Unlock()

return newCount

// easy to miss unlocks
// due to multiple returns
 
p.Lock()
defer p.Unlock()

if p.count < 10 {
  return p.count
}

p.count++
return p.count

// more readable

Defer has an extremely small overhead and should be avoided only if you can prove that your function execution time is in the order of nanoseconds. The readability win of using defers is worth the minuscule cost of using them. This is especially true for larger methods that have more than simple memory accesses, where the other computations are more significant than the defer.

Channel Size is One or None

Guideline Identifier: ChannelSizeIsOneOrNone

Channels should usually have a size of one or be unbuffered. By default, channels are unbuffered and have a size of zero. Any other size must be subject to a high level of scrutiny. Consider how the size is determined, what prevents the channel from filling up under load and blocking writers, and what happens when this occurs.

Not Recommended Good 
// Ought to be enough for anybody!
c := make(chan int, 64)
 
// Size of one
c := make(chan int, 1) // or
// Unbuffered channel, size of zero
c := make(chan int)

Start Enums at One

Guideline Identifier: StartEnumsAtOne

The standard way of introducing enumerations in Go is to declare a custom type and a const group with iota. Since variables have a 0 default value, you should usually start your enums on a non-zero value.

Not Recommended Good 
type Operation int

const (
  Add Operation = iota
  Subtract
  Multiply
)

// Add=0, Subtract=1, Multiply=2
 
type Operation int

const (
  Add Operation = iota + 1
  Subtract
  Multiply
)

// Add=1, Subtract=2, Multiply=3

Another common pattern that achieves the same result (starting enums at one) is to assign the first iota value in the constant block to a _ or to a constant that indicates the value is invalid:

type Operation int

const (
  _ Operation = iota
  Add
  Subtract
  Multiply
)

// Add=1, Subtract=2, Multiply=3

There are cases where using the zero value makes sense, for example when the zero value case is the desirable default behavior.

type LogOutput int

const (
  LogToStdout LogOutput = iota
  LogToFile
  LogToRemote
)

// LogToStdout=0, LogToFile=1, LogToRemote=2

Use "time" to handle time

Guideline Identifier: UseTimeToHandleTime

Time is complicated. Incorrect assumptions often made about time include the following.

  • A day has 24 hours
  • An hour has 60 minutes
  • A week has 7 days
  • A year has 365 days
  • And a lot more

For example, 1 means that adding 24 hours to a time instant will not always yield a new calendar day.

Therefore, always use the "time" package when dealing with time because it helps deal with these incorrect assumptions in a safer, more accurate manner.

Use time.Time for instants of time

Guideline Identifier: UseTimeToHandleTime_TimeForInstantsOfTime

Use time.Time when dealing with instants of time, and the methods on time.Time when comparing, adding, or subtracting time.

Not Recommended Good 
func isActive(now, start, stop int) bool {
  return start <= now && now < stop
}
 
func isActive(now, start, stop time.Time) bool {
  return (start.Before(now) || start.Equal(now)) &&
      now.Before(stop)
}

Use time.Duration for periods of time

Guideline Identifier: UseTimeToHandleTime_DurationForPeriodsOfTime

Use time.Duration when dealing with periods of time.

Not Recommended Good 
func poll(delay int) {
  for {
    // ...
    time.Sleep(time.Duration(delay) * time.Millisecond)
  }
}

poll(10) // was it seconds or milliseconds?
 
func poll(delay time.Duration) {
  for {
    // ...
    time.Sleep(delay)
  }
}

poll(10*time.Second)

Going back to the example of adding 24 hours to a time instant, the method we use to add time depends on intent. If we want the same time of the day, but on the next calendar day, we should use Time.AddDate. However, if we want an instant of time guaranteed to be 24 hours after the previous time, we should use Time.Add.

newDay := t.AddDate(0 /* years */, 0 /* months */, 1 /* days */)
maybeNewDay := t.Add(24 * time.Hour)

Use time.Time and time.Duration with external systems

Guideline Identifier: UseTimeToHandleTime_TimeAndDurationWithExternalSystems

Use time.Duration and time.Time in interactions with external systems when possible. For example:

When it is not possible to use time.Duration in these interactions, use int or float64 and include the unit in the name of the field.

For example, since encoding/json does not support time.Duration, the unit is included in the name of the field.

Not Recommended Good 
// {"interval": 2}
type Config struct {
  Interval int `json:"interval"`
}
 
// {"intervalMillis": 2000}
type Config struct {
  IntervalMillis int `json:"intervalMillis"`
}

When it is not possible to use time.Time in these interactions, unless an alternative is agreed upon, use string and format timestamps as defined in RFC 3339. This format is used by default by Time.UnmarshalText and is available for use in Time.Format and time.Parse via time.RFC3339.

Although this tends to not be a problem in practice, keep in mind that the "time" package does not support parsing timestamps with leap seconds (8728), nor does it account for leap seconds in calculations (15190). If you compare two instants of time, the difference will not include the leap seconds that may have occurred between those two instants.

Errors

Note that we removed the use of the now-deprecated github.com/pkg/errors package, and switched to using the errors package available in the standard library. See https://github.com/redhat-developer/odo/pull/5557

Error Types

Guideline Identifier: Errors_ErrorTypes

There are few options for declaring errors. Consider the following before picking the option best suited for your use case.

  • Does the caller need to match the error so that they can handle it? If yes, we must support the errors.Is or errors.As functions by declaring a top-level error variable or a custom type.
  • Is the error message a static string, or is it a dynamic string that requires contextual information? For the former, we can use errors.New, but for the latter we must use fmt.Errorf or a custom error type.
  • Are we propagating a new error returned by a downstream function? If so, see the section on error wrapping.
Error matching? Error Message Guidance
No static errors.New
No dynamic fmt.Errorf
Yes static top-level var with errors.New
Yes dynamic custom error type

For example, use errors.New for an error with a static string. Export this error as a variable to support matching it with errors.Is if the caller needs to match and handle this error.

No error matching Error matching 
// package foo

func Open() error {
  return errors.New("could not open")
}

// package bar

if err := foo.Open(); err != nil {
  // Can't handle the error.
  panic("unknown error")
}
 
// package foo

var ErrCouldNotOpen = errors.New("could not open")

func Open() error {
  return ErrCouldNotOpen
}

// package bar

if err := foo.Open(); err != nil {
  if errors.Is(err, foo.ErrCouldNotOpen) {
    // handle the error
  } else {
    panic("unknown error")
  }
}

For an error with a dynamic string, use fmt.Errorf if the caller does not need to match it, and a custom error if the caller does need to match it.

No error matching Error matching 
// package foo

func Open(file string) error {
  return fmt.Errorf("file %q not found", file)
}

// package bar

if err := foo.Open("testfile.txt"); err != nil {
  // Can't handle the error.
  panic("unknown error")
}
 
// package foo

type NotFoundError struct {
  File string
}

func (e *NotFoundError) Error() string {
  return fmt.Sprintf("file %q not found", e.File)
}

func Open(file string) error {
  return &NotFoundError{File: file}
}


// package bar

if err := foo.Open("testfile.txt"); err != nil {
  var notFound *NotFoundError
  if errors.As(err, &notFound) {
    // handle the error
  } else {
    panic("unknown error")
  }
}

Note that if you export error variables or types from a package, they will become part of the public API of the package.

Checking Error Types

Guideline Identifier: Errors_CheckingErrorTypes

  • To check error equality don’t use ==. Use errors.Is instead (for Go versions >= 1.13).
  • To check if the error is of a certain type, don’t use type assertion, use errors.As instead (for Go versions >= 1.13).

Error Wrapping

Guideline Identifier: Errors_ErrorWrapping

Bear in mind that we have a requirement of being able to return error stacks to plugin authors, e.g., in a serialized form like JSON. So adding context to errors could be helpful.

There are essentially three main options for propagating errors if a call fails:

  • return the original error as-is
  • add context with fmt.Errorf and the %w verb
  • add context with fmt.Errorf and the %v verb

Return the original error as-is if there is no additional context to add. This maintains the original error type and message. This is well suited for cases when the underlying error message has sufficient information to track down where it came from.

Otherwise, add context to the error message where possible so that instead of a vague error such as "connection refused", you get more useful errors such as "call service foo: connection refused".

Use fmt.Errorf to add context to your errors, picking between the %w or %v verbs based on whether the caller should be able to match and extract the underlying cause.

  • Use %w if the caller should have access to the underlying error. This is a good default for most wrapped errors, but be aware that callers may begin to rely on this behavior. So for cases where the wrapped error is a known var or type, document and test it as part of your function's contract.
  • Use %v to obfuscate the underlying error. Callers will be unable to match it, but you can switch to %w in the future if needed.

When adding context to returned errors, keep the context succinct by avoiding phrases like "failed to", which state the obvious and pile up as the error percolates up through the stack:

Not Recommended Good 
s, err := store.New()
if err != nil {
    return fmt.Errorf(
        "failed to create new store: %w", err)
}
 
s, err := store.New()
if err != nil {
    return fmt.Errorf(
        "new store: %w", err)
}
 
failed to x: failed to y: failed to create new store: the error

x: y: new store: the error

However once the error is sent to another system, it should be clear the message is an error (e.g. an err tag or "Failed" prefix in logs).

See also Don't just check errors, handle them gracefully.

Error Naming

Guideline Identifier: Errors_ErrorNaming

For error values stored as global variables, use the prefix Err or err depending on whether they're exported. This guidance supersedes the Prefix Unexported Globals with _.

var (
  // The following two errors are exported
  // so that users of this package can match them
  // with errors.Is.

  ErrBrokenLink = errors.New("link is broken")
  ErrCouldNotOpen = errors.New("could not open")

  // This error is not exported because
  // we don't want to make it part of our public API.
  // We may still use it inside the package
  // with errors.Is.

  errNotFound = errors.New("not found")
)

For custom error types, use the suffix Error instead.

// Similarly, this error is exported
// so that users of this package can match it
// with errors.As.

type NotFoundError struct {
  File string
}

func (e *NotFoundError) Error() string {
  return fmt.Sprintf("file %q not found", e.File)
}

// And this error is not exported because
// we don't want to make it part of the public API.
// We can still use it inside the package
// with errors.As.

type resolveError struct {
  Path string
}

func (e *resolveError) Error() string {
  return fmt.Sprintf("resolve %q", e.Path)
}

Print errors on stderr

Guideline Identifier: Errors_Stderr

If needed, errors should be written to the standard error, to make it easier for users and other tools to pipe their outputs to files or more tools.

Handle Type Assertions

Guideline Identifier: TypeAssertions

The single return value form of a type assertion will panic on an incorrect type. Therefore, always use the "comma ok" idiom.

Not Recommended Good 
t := i.(string)
 
t, ok := i.(string)
if !ok {
  // handle the error gracefully
}

Don't Panic

Guideline Identifier: DontPanic

Code running in production must avoid panics. If an error occurs, the function must return an error and allow the caller to decide how to handle it.

Not Recommended Good 
func run(args []string) {
  if len(args) == 0 {
    panic("an argument is required")
  }
  // ...
}

func main() {
  run(os.Args[1:])
}
 
func run(args []string) error {
  if len(args) == 0 {
    return errors.New("an argument is required")
  }
  // ...
  return nil
}

func main() {
  if err := run(os.Args[1:]); err != nil {
    fmt.Fprintln(os.Stderr, err)
    os.Exit(1)
  }
}

Panic/recover is not an error handling strategy. A program must panic only when something irrecoverable happens such as a nil dereference.

Few exceptions to this:

  • program initialization: bad things at program startup that should abort the program may cause panic.
  • Or when creating explicit assertions, like when intentionally calling regexp.MustCompile, which panics if the regular expression is not valid (because we know the regexp passed is a valid constant, and we purposely want the program to panic earlier if this is invalid): var _statusTemplate = template.Must(template.New("name").Parse("_statusHTML"))

Even in tests, prefer t.Fatal or t.FailNow over panics to ensure that the test is marked as failed.

Not Recommended Good 
// func TestFoo(t *testing.T)

f, err := ioutil.TempFile("", "test")
if err != nil {
  panic("failed to set up test")
}
 
// func TestFoo(t *testing.T)

f, err := ioutil.TempFile("", "test")
if err != nil {
  t.Fatal("failed to set up test")
}

Favor dependency injection over mutable globals

Guideline Identifier: DependencyInjectionOverMutableGlobals

Avoid mutating global variables, instead opting for dependency injection. This applies to function pointers as well as other kinds of values.

Mutable globals makes testing complicated, and increase the chance of introducing side effects if the package is included multiple times.

Not Recommended Good 
// sign.go

var _timeNow = time.Now

func sign(msg string) string {
  now := _timeNow()
  return signWithTime(msg, now)
}
 
// sign.go

type signer struct {
  now func() time.Time
}

func newSigner() *signer {
  return &signer{
    now: time.Now,
  }
}

func (s *signer) Sign(msg string) string {
  now := s.now()
  return signWithTime(msg, now)
}
 
// sign_test.go

func TestSign(t *testing.T) {
  oldTimeNow := _timeNow
  _timeNow = func() time.Time {
    return someFixedTime
  }
  defer func() { _timeNow = oldTimeNow }()

  assert.Equal(t, want, sign(give))
}
 
// sign_test.go

func TestSigner(t *testing.T) {
  s := newSigner()
  s.now = func() time.Time {
    return someFixedTime
  }

  assert.Equal(t, want, s.Sign(give))
}

Avoid Embedding Types in Public Structs

Guideline Identifier: EmbeddingTypesInPublicStructs

These embedded types leak implementation details, inhibit type evolution, and obscure documentation.

Assuming you have implemented a variety of list types using a shared AbstractList, avoid embedding the AbstractList in your concrete list implementations. Instead, hand-write only the methods to your concrete list that will delegate to the abstract list.

type AbstractList struct {}

// Add adds an entity to the list.
func (l *AbstractList) Add(e Entity) {
  // ...
}

// Remove removes an entity from the list.
func (l *AbstractList) Remove(e Entity) {
  // ...
}
Not Recommended Good 
// ConcreteList is a list of entities.
type ConcreteList struct {
  *AbstractList
}
 
// ConcreteList is a list of entities.
type ConcreteList struct {
  list *AbstractList
}

// Add adds an entity to the list.
func (l *ConcreteList) Add(e Entity) {
  l.list.Add(e)
}

// Remove removes an entity from the list.
func (l *ConcreteList) Remove(e Entity) {
  l.list.Remove(e)
}

Go allows type embedding as a compromise between inheritance and composition. The outer type gets implicit copies of the embedded type's methods. These methods, by default, delegate to the same method of the embedded instance.

The struct also gains a field by the same name as the type. So, if the embedded type is public, the field is public. To maintain backward compatibility, every future version of the outer type must keep the embedded type.

An embedded type is rarely necessary. It is a convenience that helps you avoid writing tedious delegate methods.

Even embedding a compatible AbstractList interface, instead of the struct, would offer the developer more flexibility to change in the future, but still leak the detail that the concrete lists use an abstract implementation.

Not Recommended Good 
// AbstractList is a generalized implementation
// for various kinds of lists of entities.
type AbstractList interface {
  Add(Entity)
  Remove(Entity)
}

// ConcreteList is a list of entities.
type ConcreteList struct {
  AbstractList
}
 
// ConcreteList is a list of entities.
type ConcreteList struct {
  list AbstractList
}

// Add adds an entity to the list.
func (l *ConcreteList) Add(e Entity) {
  l.list.Add(e)
}

// Remove removes an entity from the list.
func (l *ConcreteList) Remove(e Entity) {
  l.list.Remove(e)
}

Either with an embedded struct or an embedded interface, the embedded type places limits on the evolution of the type.

  • Adding methods to an embedded interface is a breaking change.
  • Removing methods from an embedded struct is a breaking change.
  • Removing the embedded type is a breaking change.
  • Replacing the embedded type, even with an alternative that satisfies the same interface, is a breaking change.

Although writing these delegate methods is tedious, the additional effort hides an implementation detail, leaves more opportunities for change, and also eliminates indirection for discovering the full List interface in documentation.

Avoid Using Built-In Names

Guideline Identifier: BuiltInNames

The Go language specification outlines several built-in, predeclared identifiers that should not be used as names within Go programs.

Depending on context, reusing these identifiers as names will either shadow the original within the current lexical scope (and any nested scopes) or make affected code confusing. In the best case, the compiler will complain; in the worst case, such code may introduce latent, hard-to-grep bugs.

Not Recommended Good 
var error string
// `error` shadows the builtin

// or

func handleErrorMessage(error string) {
    // `error` shadows the builtin
}
 
var errorMessage string
// `error` refers to the builtin

// or

func handleErrorMessage(msg string) {
    // `error` refers to the builtin
}
 
type Foo struct {
    // While these fields technically don't
    // constitute shadowing, grepping for
    // `error` or `string` strings is now
    // ambiguous.
    error  error
    string string
}

func (f Foo) Error() error {
    // `error` and `f.error` are
    // visually similar
    return f.error
}

func (f Foo) String() string {
    // `string` and `f.string` are
    // visually similar
    return f.string
}
 
type Foo struct {
    // `error` and `string` strings are
    // now unambiguous.
    err error
    str string
}

func (f Foo) Error() error {
    return f.err
}

func (f Foo) String() string {
    return f.str
}

Note that the compiler will not generate errors when using predeclared identifiers, but tools such as go vet should correctly point out these and other cases of shadowing.

Avoid init()

Guideline Identifier: Avoid_Init

Avoid init() where possible. When init() is unavoidable or desirable, code should attempt to:

  1. Be completely deterministic, regardless of program environment or invocation.
  2. Avoid depending on the ordering or side-effects of other init() functions. While init() ordering is well-known, code can change, and thus relationships between init() functions can make code brittle and error-prone.
  3. Avoid accessing or manipulating global or environment state, such as machine information, environment variables, working directory, program arguments/inputs, etc.
  4. Avoid I/O, including both filesystem, network, and system calls.

Code that cannot satisfy these requirements likely belongs as a helper to be called as part of main() (or elsewhere in a program's lifecycle), or be written as part of main() itself. In particular, libraries that are intended to be used by other programs should take special care to be completely deterministic and not perform "init magic".

Not Recommended Good 
type Foo struct {
    // ...
}

var _defaultFoo Foo

func init() {
    _defaultFoo = Foo{
        // ...
    }
}
 
var _defaultFoo = Foo{
    // ...
}

// or, better, for testability:

var _defaultFoo = defaultFoo()

func defaultFoo() Foo {
    return Foo{
        // ...
    }
}
 
type Config struct {
    // ...
}

var _config Config

func init() {
    // Bad: based on current directory
    cwd, _ := os.Getwd()

    // Bad: I/O
    raw, _ := ioutil.ReadFile(
        path.Join(cwd, "config", "config.yaml"),
    )

    yaml.Unmarshal(raw, &_config)
}
 
type Config struct {
    // ...
}

func loadConfig() Config {
    cwd, err := os.Getwd()
    // handle err

    raw, err := ioutil.ReadFile(
        path.Join(cwd, "config", "config.yaml"),
    )
    // handle err

    var config Config
    yaml.Unmarshal(raw, &config)

    return config
}

Considering the above, some situations in which init() may be preferable or necessary might include:

  • Complex expressions that cannot be represented as single assignments.
  • Pluggable hooks, such as database/sql dialects, encoding type registries, etc.
  • Optimizations to Google Cloud Functions and other forms of deterministic precomputation.

Exit in Main

Guideline Identifier: ExitInMain

Go programs use os.Exit or log.Fatal* to exit immediately. (Panicking is not a good way to exit programs, please don't panic.)

Call one of os.Exit or log.Fatal* only in main(). All other functions should return errors to signal failure.

Not Recommended Good 
func main() {
  body := readFile(path)
  fmt.Println(body)
}

func readFile(path string) string {
  f, err := os.Open(path)
  if err != nil {
    log.Fatal(err)
  }

  b, err := ioutil.ReadAll(f)
  if err != nil {
    log.Fatal(err)
  }

  return string(b)
}
 
func main() {
  body, err := readFile(path)
  if err != nil {
    log.Fatal(err)
  }
  fmt.Println(body)
}

func readFile(path string) (string, error) {
  f, err := os.Open(path)
  if err != nil {
    return "", err
  }

  b, err := ioutil.ReadAll(f)
  if err != nil {
    return "", err
  }

  return string(b), nil
}

Rationale: Programs with multiple functions that exit present a few issues:

  • Non-obvious control flow: Any function can exit the program so it becomes difficult to reason about the control flow.
  • Difficult to test: A function that exits the program will also exit the test calling it. This makes the function difficult to test and introduces risk of skipping other tests that have not yet been run by go test.
  • Skipped cleanup: When a function exits the program, it skips function calls enqueued with defer statements. This adds risk of skipping important cleanup tasks.

Exit Once

Guideline Identifier: ExitInMain_ExitOnce

If possible, prefer to call os.Exit or log.Fatal at most once in your main(). If there are multiple error scenarios that halt program execution, put that logic under a separate function and return errors from it.

This has the effect of shortening your main() function and putting all key business logic into a separate, testable function.

Not Recommended Good 
package main

func main() {
  args := os.Args[1:]
  if len(args) != 1 {
    log.Fatal("missing file")
  }
  name := args[0]

  f, err := os.Open(name)
  if err != nil {
    log.Fatal(err)
  }
  defer f.Close()

  // If we call log.Fatal after this line,
  // f.Close will not be called.

  b, err := ioutil.ReadAll(f)
  if err != nil {
    log.Fatal(err)
  }

  // ...
}
 
package main

func main() {
  if err := run(); err != nil {
    log.Fatal(err)
  }
}

func run() error {
  args := os.Args[1:]
  if len(args) != 1 {
    return errors.New("missing file")
  }
  name := args[0]

  f, err := os.Open(name)
  if err != nil {
    return err
  }
  defer f.Close()

  b, err := ioutil.ReadAll(f)
  if err != nil {
    return err
  }

  // ...
}

Use field tags in marshaled structs

Guideline Identifier: FieldTagsInMarshaledStructs

Any struct field that is marshaled into JSON, YAML, or other formats that support tag-based field naming should be annotated with the relevant tag.

Not Recommended Good 
type Stock struct {
  Price int
  Name  string
}

bytes, err := json.Marshal(Stock{
  Price: 137,
  Name:  "UBER",
})
 
type Stock struct {
  Price int    `json:"price"`
  Name  string `json:"name"`
  // Safe to rename Name to Symbol.
}

bytes, err := json.Marshal(Stock{
  Price: 137,
  Name:  "UBER",
})

Rationale: The serialized form of the structure is a contract between different systems. Changes to the structure of the serialized form--including field names--break this contract. Specifying field names inside tags makes the contract explicit, and it guards against accidentally breaking the contract by refactoring or renaming fields.

Prefer strconv over fmt

Guideline Identifier: StrconvOverFmt

When converting primitives to/from strings, strconv is faster than fmt.

Not Recommended Good 
for i := 0; i < b.N; i++ {
  s := fmt.Sprint(rand.Int())
}
 
for i := 0; i < b.N; i++ {
  s := strconv.Itoa(rand.Int())
}
 
BenchmarkFmtSprint-4    143 ns/op    2 allocs/op

BenchmarkStrconv-4    64.2 ns/op    1 allocs/op

Avoid string-to-byte conversion

Guideline Identifier: StringToByteConversion

Do not create byte slices from a fixed string repeatedly. Instead, perform the conversion once and capture the result.

Not Recommended Good 
for i := 0; i < b.N; i++ {
  w.Write([]byte("Hello world"))
}
 
data := []byte("Hello world")
for i := 0; i < b.N; i++ {
  w.Write(data)
}
 
BenchmarkBad-4   50000000   22.2 ns/op

BenchmarkGood-4  500000000   3.25 ns/op

Prefer strings.Builder to efficiently build and concatenate strings

Guideline Identifier: StringBuildersToBuildAndConcatenateStrings

strings.Builder is used to efficiently build a string using Write methods. It minimizes memory copying. The zero value is ready to use. Do not copy a non-zero Builder. Note that strings.Builder also implements the io.Writer interface, which means we can use functions like fmt.Fprintf along with the string builder.

Prefer Specifying Container Capacity

Guideline Identifier: ContainerCapacity

Specify container capacity where possible in order to allocate memory for the container up front. This minimizes subsequent allocations (by copying and resizing of the container) as elements are added.

Specifying Map Capacity Hints

Guideline Identifier: ContainerCapacity_MapCapacityHints

Where possible, provide capacity hints when initializing maps with make().

make(map[T1]T2, hint)

Providing a capacity hint to make() tries to right-size the map at initialization time, which reduces the need for growing the map and allocations as elements are added to the map.

Note that, unlike slices, map capacity hints do not guarantee complete, preemptive allocation, but are used to approximate the number of hashmap buckets required. Consequently, allocations may still occur when adding elements to the map, even up to the specified capacity.

Not Recommended Good 
m := make(map[string]os.FileInfo)

files, _ := ioutil.ReadDir("./files")
for _, f := range files {
    m[f.Name()] = f
}
 
files, _ := ioutil.ReadDir("./files")

m := make(map[string]os.FileInfo, len(files))
for _, f := range files {
    m[f.Name()] = f
}

m is created without a size hint; there may be more allocations at assignment time.

m is created with a size hint; there may be fewer allocations at assignment time.

Specifying Slice Capacity

Guideline Identifier: ContainerCapacity_SliceCapacity

Where possible, provide capacity hints when initializing slices with make(), particularly when appending.

make([]T, length, capacity)

Unlike maps, slice capacity is not a hint: the compiler will allocate enough memory for the capacity of the slice as provided to make(), which means that subsequent append() operations will incur zero allocations (until the length of the slice matches the capacity, after which any appends will require a resize to hold additional elements).

Not Recommended Good 
for n := 0; n < b.N; n++ {
  data := make([]int, 0)
  for k := 0; k < size; k++{
    data = append(data, k)
  }
}
 
for n := 0; n < b.N; n++ {
  data := make([]int, 0, size)
  for k := 0; k < size; k++{
    //append is mostly used in `odo`
    data = append(data, k)
    //Another possibility to keep in mind
    data[k] = k
  }
}
 
BenchmarkBad-4    100000000    2.48s

BenchmarkGood-4   100000000    0.21s

Note: The odo codebase mostly uses append.

Avoid global variables, even in packages

Guideline Identifier: GlobalVariables

Global variables increase the chance of introducing side effects if the package is included multiple times. It also makes testing complicated. Exception: Singletons

Avoid overly long lines

Guideline Identifier: OverlyLongLines

Avoid lines of code that require readers to scroll horizontally or turn their heads too much. Overly long lines of code might also cause PR to take more time to review.

We recommend a soft line length limit of 99 characters. Authors should aim to wrap lines before hitting this limit, but it is not a hard limit. Code is allowed to exceed this limit.

Tooling suggestion:

Be Consistent

Guideline Identifier: StyleConsistency

Some of the guidelines outlined in this document can be evaluated objectively; others are situational, contextual, or subjective.

Above all else, be consistent.

Consistent code is easier to maintain, is easier to rationalize, requires less cognitive overhead, and is easier to migrate or update as new conventions emerge or classes of bugs are fixed.

Conversely, having multiple disparate or conflicting styles within a single codebase causes maintenance overhead, uncertainty, and cognitive dissonance, all of which can directly contribute to lower velocity, painful code reviews, and bugs.

When applying these guidelines to a codebase, it is recommended that changes are made at a package (or larger) level: application at a sub-package level violates the above concern by introducing multiple styles into the same code.

Group Similar Declarations

Guideline Identifier: SimilarDeclarationsGrouping

Go supports grouping similar declarations.

Not Recommended Good 
import "a"
import "b"
 
import (
  "a"
  "b"
)

This also applies to constants, variables, and type declarations.

Not Recommended Good 
const a = 1
const b = 2



var a = 1
var b = 2



type Area float64
type Volume float64
 
const (
  a = 1
  b = 2
)

var (
  a = 1
  b = 2
)

type (
  Area float64
  Volume float64
)

Only group related declarations. Do not group declarations that are unrelated.

Not Recommended Good 
type Operation int

const (
  Add Operation = iota + 1
  Subtract
  Multiply
  EnvVar = "MY_ENV"
)
 
type Operation int

const (
  Add Operation = iota + 1
  Subtract
  Multiply
)

const EnvVar = "MY_ENV"

Groups are not limited in where they can be used. For example, you can use them inside of functions.

Not Recommended Good 
func f() string {
  red := color.New(0xff0000)
  green := color.New(0x00ff00)
  blue := color.New(0x0000ff)

  // ...
}
 
func f() string {
  var (
    red   = color.New(0xff0000)
    green = color.New(0x00ff00)
    blue  = color.New(0x0000ff)
  )

  // ...
}

Exception: Variable declarations, particularly inside functions, should be grouped together if declared adjacent to other variables. Do this for variables declared together even if they are unrelated.

Not Recommended Good 
func (c *client) request() {
  caller := c.name
  format := "json"
  timeout := 5*time.Second
  var err error

  // ...
}
 
func (c *client) request() {
  var (
    caller  = c.name
    format  = "json"
    timeout = 5*time.Second
    err error
  )

  // ...
}

Import Group Ordering

Guideline Identifier: GroupOrdering

We recommend using the following import groups:

  • Standard library
  • Devfile
  • odo
  • k8s.io
  • Everything else

Tooling

  • goimports applies this by putting standard library packages first. But it should be possible to configure it differently. We recommend setting up your editor to run goimports automatically whenever you save files in the odo repository. More information on editor support for Go tools here: https://github.com/golang/go/wiki/IDEsAndTextEditorPlugins

TODO

  • Tooling with golangci-lint
  • Existing code to fix (as of 2022-04-04)
❯ goimports -l $(find . -type f -name '*.go' -not -path "./vendor/*")
./pkg/testingutil/devfile.go
./pkg/service/link.go
./pkg/odo/cli/telemetry/telemetry.go
./pkg/odo/cli/login/login_test.go
./pkg/odo/cli/project/create.go
./tests/helper/helper_telemetry.go
./tests/helper/helper_generic.go

Dependencies

Dependencies, whether internal or external, should ideally be kept to the minimum.

External Dependencies

Guideline Identifier: Dependencies_External

The introduction of a new dependency should be argued in the Pull Request. Reviewers and maintainers should ensure new dependencies’ security status and license compatibility. This is enforced with wwhrd - see https://github.com/redhat-developer/odo/wiki/Dev:-odo-Dev-Guidelines#licenses

Also, we should make sure that if a dependency is removed, it would never be introduced again, by updating the golang-ci.yaml configuration.

Internal Dependencies

Guideline Identifier: Dependencies_Internal

Prefer using dependency injection internally to facilitate mocking and testing.

Naming Conventions

MixedCaps

Guideline Identifier: Naming_MixedCaps

Use UpperMixedCaps or upperMixedCaps rather than underscores to write multi-word names.

Exception for test functions, which may contain underscores for grouping related test cases.

Package Names

Guideline Identifier: Naming_Packages

When naming packages, choose a name that is:

  • All lower-case. No capitals or underscores.
  • Does not need to be renamed using named imports at most call sites.
  • Short and succinct. Remember that the name is identified in full at every call site.
  • Not plural. For example, net/url, not net/urls.
  • Not "common", "util", "shared", or "lib". These are bad, uninformative names.

See also Package Names and Style guideline for Go packages.

Interface Names

Guideline Identifier: Naming_Interfaces

From Effective Go, by convention, one-method interfaces are named by the method name plus an -er suffix or similar modification to construct an agent noun: Reader, Writer, Formatter, CloseNotifier etc.

Function Names

Guideline Identifier: Naming_Functions

We follow the Go community's convention of using MixedCaps for function names. An exception is made for test functions, which may contain underscores for the purpose of grouping related test cases, e.g., TestMyFunction_WhatIsBeingTested. If your function returns a boolean, favor a name that starts with a verb and sounds like a yes/no question, e.g.: IsValid, hasElement, …

Package-level Names

Guideline Identifier: Naming_PackageLevel Package-level symbols names should not start with the package name, as they are unnecessarily redundant, when using exported symbols elsewhere.

Not Recommended Good 
package dev

type DevOptions struct {
	// ...
}
 
package dev

type Options struct {
	// ...
}

Import Aliasing

Guideline Identifier: ImportAliasing

Import aliasing must be used if the package name does not match the last element of the import path.

import (
  "net/http"

  client "example.com/client-go"
  trace "example.com/trace/v2"
)

In all other scenarios, import aliases should be avoided unless there is a direct conflict between imports.

Not Recommended Good 
import (
  "fmt"
  "os"


  nettrace "golang.net/x/trace"
)
 
import (
  "fmt"
  "os"
  "runtime/trace"

  nettrace "golang.net/x/trace"
)

Function Grouping and Ordering

Guideline Identifier: FunctionGroupingAndOrdering

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

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

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

Since functions are grouped by receiver, plain utility functions should appear towards the end of the file.

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

type something struct{ ... }

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

func (s *something) Stop() {...}

func newSomething() *something {
    return &something{}
}
 
type something struct{ ... }

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

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

func (s *something) Stop() {...}

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

Reduce Nesting

Guideline Identifier: Nesting

Code should reduce nesting where possible by handling error cases/special conditions first and returning early or continuing the loop. Reduce the amount of code that is nested multiple levels.

Not Recommended Good 
for _, v := range data {
  if v.F1 == 1 {
    v = process(v)
    if err := v.Call(); err == nil {
      v.Send()
    } else {
      return err
    }
  } else {
    log.Printf("Invalid v: %v", v)
  }
}
 
for _, v := range data {
  if v.F1 != 1 {
    log.Printf("Invalid v: %v", v)
    continue
  }

  v = process(v)
  if err := v.Call(); err != nil {
    return err
  }
  v.Send()
}

Unnecessary Else

Guideline Identifier: UnnecessaryElse

If a variable is set in both branches of an if, it can be replaced with a single if.

Not Recommended Good 
var a int
if b {
  a = 100
} else {
  a = 10
}
 
a := 10
if b {
  a = 100
}

Top-level Variable Declarations

Guideline Identifier: TopLevelVariableDeclarations

At the top level, use the standard var keyword. Do not specify the type, unless it is not the same type as the expression.

Not Recommended Good 
var _s string = F()

func F() string { return "A" }
 
var _s = F()
// Since F already states that it returns a string,
// we don't need to specify the type again.

func F() string { return "A" }

Specify the type if the type of the expression does not match the desired type exactly.

type myError struct{}

func (myError) Error() string { return "error" }

func F() myError { return myError{} }

var _e error = F()
// F returns an object of type myError but we want error.

Prefix Unexported Globals with _

Guideline Identifier: UnderscorePrefixForUnexportedGlobals

Prefix unexported top-level vars and consts with _ to make it clear when they are used that they are global symbols.

Rationale: Top-level variables and constants have a package scope. Using a generic name makes it easy to accidentally use the wrong value in a different file.

Not Recommended Good 
// foo.go

const (
  defaultPort = 8080
  defaultUser = "user"
)

// bar.go

func Bar() {
  defaultPort := 9090
  ...
  fmt.Println("Default port", defaultPort)

  // We will not see a compile error if the first line of
  // Bar() is deleted.
}
 
// foo.go

const (
  _defaultPort = 8080
  _defaultUser = "user"
)

Exception: Unexported error values may use the prefix err without the underscore. See Error Naming.

Embedding in Structs

Guideline Identifier: EmbeddingInStructs

Embedded types should be at the top of the field list of a struct, and there must be an empty line separating embedded fields from regular fields.

Not Recommended Good 
type Client struct {
  version int
  http.Client
}
 
type Client struct {
  http.Client

  version int
}

Embedding should provide tangible benefit, like adding or augmenting functionality in a semantically-appropriate way. It should do this with zero adverse user-facing effects (see also: Avoid Embedding Types in Public Structs).

Exception: Mutexes should not be embedded, even on unexported types. See also: Zero-value Mutexes are Valid.

Embedding should not:

  • Be purely cosmetic or convenience-oriented.
  • Make outer types more difficult to construct or use.
  • Affect outer types' zero values. If the outer type has a useful zero value, it should still have a useful zero value after embedding the inner type.
  • Expose unrelated functions or fields from the outer type as a side-effect of embedding the inner type.
  • Expose unexported types.
  • Affect outer types' copy semantics.
  • Change the outer type's API or type semantics.
  • Embed a non-canonical form of the inner type.
  • Expose implementation details of the outer type.
  • Allow users to observe or control type internals.
  • Change the general behavior of inner functions through wrapping in a way that would reasonably surprise users.

Simply put, embed consciously and intentionally. A good litmus test is, "would all of these exported inner methods/fields be added directly to the outer type"; if the answer is "some" or "no", don't embed the inner type - use a field instead.

Not Recommended Good 
type A struct {
    // Bad: A.Lock() and A.Unlock() are
    //      now available, provide no
    //      functional benefit, and allow
    //      users to control details about
    //      the internals of A.
    sync.Mutex
}
 
type countingWriteCloser struct {
    // Good: Write() is provided at this
    //       outer layer for a specific
    //       purpose, and delegates work
    //       to the inner type's Write().
    io.WriteCloser

    count int
}

func (w *countingWriteCloser) Write(
    bs []byte) (int, error) {
    w.count += len(bs)
    return w.WriteCloser.Write(bs)
}
 
type Book struct {
    // Bad: pointer changes zero value
    // usefulness
    io.ReadWriter

    // other fields
}

// later

var b Book
b.Read(...)  // panic: nil pointer
b.String()   // panic: nil pointer
b.Write(...) // panic: nil pointer
 
type Book struct {
    // Good: has useful zero value
    bytes.Buffer

    // other fields
}

// later

var b Book
b.Read(...)  // ok
b.String()   // ok
b.Write(...) // ok
 
type Client struct {
    sync.Mutex
    sync.WaitGroup
    bytes.Buffer
    url.URL
}
 
type Client struct {
    mtx sync.Mutex
    wg  sync.WaitGroup
    buf bytes.Buffer
    url url.URL
}

Local Variable Declarations

Guideline Identifier: LocalVariableDeclarations

Short variable declarations (:=) should be used if a variable is being set to some value explicitly.

Not Recommended Good 
var s = "foo"
 
s := "foo"

However, there are cases where the default value is clearer when the var keyword is used. Declaring Empty Slices, for example.

Not Recommended Good 
func f(list []int) {
  filtered := []int{}
  for _, v := range list {
    if v > 10 {
      filtered = append(filtered, v)
    }
  }
}
 
func f(list []int) {
  var filtered []int
  for _, v := range list {
    if v > 10 {
      filtered = append(filtered, v)
    }
  }
}

nil is a valid slice

Guideline Identifier: NilIsAValidSlice

nil is a valid slice of length 0. This means that,

  • You should not return a slice of length zero explicitly. Return nil instead.

    Not Recommended Good 
    if x == "" {
  return []int{}
}
    		 
    if x == "" {
  return nil
}

  • To check if a slice is empty, always use len(s) == 0. Do not check for nil.

    Not Recommended Good 
    func isEmpty(s []string) bool {
  return s == nil
}
    		 
    func isEmpty(s []string) bool {
  return len(s) == 0
}

  • The zero value (a slice declared with var) is usable immediately without make().

    Not Recommended Good 
    nums := []int{}
// or, nums := make([]int)

if add1 {
  nums = append(nums, 1)
}

if add2 {
  nums = append(nums, 2)
}
    		 
    var nums []int

if add1 {
  nums = append(nums, 1)
}

if add2 {
  nums = append(nums, 2)
}

Remember that, while it is a valid slice, a nil slice is not equivalent to an allocated slice of length 0 - one is nil and the other is not - and the two may be treated differently in different situations (such as serialization).

Comments

Guideline Identifier: Comments

Documentation should be taken seriously in a code base, so it can be accessible and maintainable. It should ideally be coupled to the code itself so the documentation evolves along with the code.

The convention is simple: to document a type, variable, constant, function, or even a package, write a regular comment directly preceding its declaration, with no intervening blank line. Godoc will then present that comment as text alongside the item it documents. For example, this is the documentation for the fmt package’s Fprint function:

// Fprint formats using the default formats for its operands and writes to w.
// Spaces are added between operands when neither is a string.
// It returns the number of bytes written and any write error encountered.
func Fprint(w io.Writer, a ...interface{}) (n int, err error) {

Notice this comment is a complete sentence that begins with the name of the element it describes.

Package Doc

Guideline Identifier: Comments_PackageDoc

Comments on package declarations should provide general package documentation. These comments can be short, like the sort package’s brief description:

// Package sort provides primitives for sorting slices and user-defined
// collections.
package sort

They can also be detailed like the gob package’s overview. That package uses another convention for packages that need large amounts of introductory documentation: the package comment is placed in its own file, doc.go, which contains only those comments and a package clause.

When writing package comments of any size, keep in mind that their first sentence will appear in godoc’s package list.

Documenting known bugs

Guideline Identifier: Comments_KnownBugs

Comments that are not adjacent to a top-level declaration are omitted from godoc’s output, with one notable exception. Top-level comments that begin with the word BUG(who) are recognized as known bugs, and included in the “Bugs” section of the package documentation. The “who” part should be the username of someone who could provide more information. For example, this is a known issue from the bytes package:

// BUG(r): The rule Title uses for word boundaries does not handle Unicode punctuation properly.

Documenting deprecated things

Guideline Identifier: Comments_Depreciation

Sometimes a struct field, function, type, or even a whole package becomes redundant or unnecessary, but must be kept for compatibility with existing programs. To signal that an identifier should not be used, add a paragraph to its doc comment that begins with Deprecated: followed by some information about the deprecation.

Avoid meaningless packages like utils or common

Guideline Identifier: MeaninglessPackages

Writing a good Go package starts with its name. Think of your package’s name as an elevator pitch, you have to describe what it does using just one word.

Name your packages after what they provide, not what they contain.

Reduce Scope of Variables

Guideline Identifier: ScopeOfVariables

Reduce Scope of Variables

Where possible, reduce scope of variables. Do not reduce the scope if it conflicts with Reduce Nesting.

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

If you need a result of a function call outside of the if, then you should not try to reduce the scope.

Not Recommended Good 
if data, err := ioutil.ReadFile(name); err == nil {
  err = cfg.Decode(data)
  if err != nil {
    return err
  }

  fmt.Println(cfg)
  return nil
} else {
  return err
}
 
data, err := ioutil.ReadFile(name)
if err != nil {
   return err
}

if err := cfg.Decode(data); err != nil {
  return err
}

fmt.Println(cfg)
return nil

Avoid Naked Parameters

Guideline Identifier: NakedParameters

Naked parameters in function calls can hurt readability. Add C-style comments (/* ... */) for parameter names when their meaning is not obvious.

Not Recommended Good 
// func printInfo(name string, isLocal, done bool)

printInfo("foo", true, true)
 
// func printInfo(name string, isLocal, done bool)

printInfo("foo", true /* isLocal */, true /* done */)

Better yet, replace naked bool types with custom types for more readable and type-safe code. This allows more than just two states (true/false) for that parameter in the future.

type Region int

const (
  UnknownRegion Region = iota
  Local
)

type Status int

const (
  StatusReady Status = iota + 1
  StatusDone
  // Maybe we will have a StatusInProgress in the future.
)

func printInfo(name string, region Region, status Status)

Use Raw String Literals to Avoid Escaping

Guideline Identifier: RawStringLiteralsToAvoidEscaping

Go supports raw string literals, which can span multiple lines and include quotes. Use these to avoid hand-escaped strings which are much harder to read.

Not Recommended Good 
wantError := "unknown name:\"test\""
 
wantError := `unknown error:"test"`

Initializing Structs

Use Field Names to Initialize Structs

Guideline Identifier: StructInitialization_FieldNames

You should almost always specify field names when initializing structs. This is now enforced by go vet.

Not Recommended Good 
k := User{"John", "Doe", true}
 
k := User{
    FirstName: "John",
    LastName: "Doe",
    Admin: true,
}

Exception: Field names may be omitted in test tables when there are 3 or fewer fields.

tests := []struct{
  op Operation
  want string
}{
  {Add, "add"},
  {Subtract, "subtract"},
}

Omit Zero Value Fields in Structs

Guideline Identifier: StructInitialization_ZeroValueFields

When initializing structs with field names, omit fields that have zero values unless they provide meaningful context. Otherwise, let Go set these to zero values automatically.

Not Recommended Good 
user := User{
  FirstName: "John",
  LastName: "Doe",
  MiddleName: "",
  Admin: false,
}
 
user := User{
  FirstName: "John",
  LastName: "Doe",
}

This helps reduce noise for readers by omitting values that are default in that context. Only meaningful values are specified.

Include zero values where field names provide meaningful context. For example, test cases in Test Tables can benefit from names of fields even when they are zero-valued.

tests := []struct{
  give string
  want int
}{
  {give: "0", want: 0},
  // ...
}

Use var for Zero Value Structs

Guideline Identifier: StructInitialization_VarForZeroValueStructs

When all the fields of a struct are omitted in a declaration, use the var form to declare the struct.

Not Recommended Good 
user := User{}
 
var user User

This differentiates zero valued structs from those with non-zero fields similar to the distinction created for map initialization, and matches how we prefer to declare empty slices.

Initializing Struct References

Guideline Identifier: StructInitialization_StructReferenceInitialization

Use &T{} instead of new(T) when initializing struct references so that it is consistent with the struct initialization.

Not Recommended Good 
sval := T{Name: "foo"}

// inconsistent
sptr := new(T)
sptr.Name = "bar"
 
sval := T{Name: "foo"}

sptr := &T{Name: "bar"}

Initializing Maps

Guideline Identifier: MapInitialization

Prefer make(..) for empty maps, and maps populated programmatically. This makes map initialization visually distinct from declaration, and it makes it easy to add size hints later if available.

Not Recommended Good 
var (
  // m1 is safe to read and write;
  // m2 will panic on writes.
  m1 = map[T1]T2{}
  m2 map[T1]T2
)
 
var (
  // m1 is safe to read and write;
  // m2 will panic on writes.
  m1 = make(map[T1]T2)
  m2 map[T1]T2
)

Declaration and initialization are visually similar.

Declaration and initialization are visually distinct.

Where possible, provide capacity hints when initializing maps with make(). See Specifying Map Capacity Hints for more information.

On the other hand, if the map holds a fixed list of elements, use map literals to initialize the map.

Not Recommended Good 
m := make(map[T1]T2, 3)
m[k1] = v1
m[k2] = v2
m[k3] = v3
 
m := map[T1]T2{
  k1: v1,
  k2: v2,
  k3: v3,
}

The basic rule of thumb is to use map literals when adding a fixed set of elements at initialization time, otherwise use make (and specify a size hint if available).

Format Strings outside Printf

Guideline Identifier: FormatStringsOutsidePrintf

If you declare format strings for Printf-style functions outside a string literal, make them const values.

This helps go vet perform static analysis of the format string.

Not Recommended Good 
msg := "unexpected values %v, %v\n"
fmt.Printf(msg, 1, 2)
 
const msg = "unexpected values %v, %v\n"
fmt.Printf(msg, 1, 2)

Naming Printf-style Functions

Guideline Identifier: VetPrintStyleFunctions

When you declare a Printf-style function, make sure that go vet can detect it and check the format string.

This means that you should use predefined Printf-style function names if possible. go vet will check these by default. See Printf family for more information.

If using the predefined names is not an option, end the name you choose with f: Wrapf, not Wrap. go vet can be asked to check specific Printf-style names but they must end with f.

$ go vet -printfuncs=wrapf,statusf

See also go vet: Printf family check.

TODO

  • Configure golangci-lint and go vet to perform such checks

Testing

Table-Driven Tests

Guideline Identifier: Testing_NumberOfFieldsInTableStruct

Pay attention to the number of fields in the table struct. Not an absolute rule, but a recommendation. Too many test cases in a same table might make the test function difficult to read. In this case, prefer creating separate test functions.

Prefer BeforeEach over JustBeforeEach

Guideline Identifier: Testing_BeforeEachOverJustBeforeEach

JustBeforeEach should be used with care as it can add complexity to a test suite.

To find out more, read https://onsi.github.io/ginkgo/#separating-creation-and-configuration-justbeforeeach

Helper functions over expectations in setup and teardown

Guideline Identifier: Testing_HelperFunctionsOverExpectationsInSetupAndTeardown

Instead of setup and teardown directly making assertions, prefer helper functions that perform such assertions. It makes the test code more readable, since most of the time when reading a test, you want to quickly get the intent.

BeforeEach should reflect what is written in the "When" description

Guideline Identifier: Testing_BeforeEachAndWhenDescription

BeforeEach allows to set up the state of our test specs, which should actually be narratively documented by the upper When description.

Code in each It should reflect what is written in the It description

Guideline Identifier: Testing_ItCodeAndDescription

To help understand a test spec, the code in it should match its description. So, pay attention when copy-pasting and customizing tests specs. Make sure to update the description accordingly.

Favor By for related tests in a same It block

Guideline Identifier: Testing_ByForRelatedTestsInSameItBlock

As a rule, you should try to keep your subject and setup closures short and to the point. Sometimes this is not possible, particularly when testing complex workflows in integration-style tests. In these cases, the test blocks begin to hide a narrative that is hard to glean by looking at code alone. Ginkgo provides By to help in these situations. Here's an example:

When("the component is deleted", func() {
  BeforeEach(func() {
    helper.Cmd("odo", "delete", "component", "-f").ShouldPass().Out()
  })

  It("should have deleted the component", func() {
    By("deleting the component", func() {
      Eventually(string(commonVar.CliRunner.Run(getDeployArgs...).Out.Contents()), 60, 3).
		  ShouldNot(ContainSubstring(cmpName))
    })

    By("deleting the deployment", func() {
      Eventually(string(commonVar.CliRunner.Run(getDeployArgs...).Out.Contents()), 60, 3).
		  ShouldNot(ContainSubstring(deploymentName))
    })

    By("deleting the service", func() {
      Eventually(string(commonVar.CliRunner.Run(getSVCArgs...).Out.Contents()), 60, 3).
		  ShouldNot(ContainSubstring(serviceName))
    })
  })
})

Consider the contract Eventually and Consistently place upon your implementation

Guideline Identifier: Testing_EventuallyAndConsistentlyContract

Use Eventually and Consistenly only for testing asynchronous code.

These utilities are extremely helpful but they are also placing a hidden constraint on your code (that some action should be time-bounded) and may hide other issues such as deadlocks.

If you find yourself bumping timeouts to appease these functions, consider whether you should be using something else.

Use goroutines in tests with care

Guideline Identifier: Testing_GoroutinesInTests

Be careful with goroutines in your tests, because they don’t play well with Ginkgo’s failure model, and if they aren’t carefully managed they can leak between tests.

Dot-import the testing libraries

Guideline Identifier: Testing_DotImportTestingLibraries

Dot imports in Go pollute the namespace with public functions and types from the imported package. Avoid doing so as much as possible. Exception: it can be useful to dot-import ginkgo, gomega and its friends gexec, gbytes and ghttp.

Don’t use the Omega symbol

Guideline Identifier: Testing_OmegaSymbol

When Gomega was first created, you could use Ω instead of Expect e.g. Ω(foo).To(Equal(bar)) This is no longer recommended. Use Expect instead.

Git

Pull Requests (PRs)

Guideline Identifier: Git_SmallPRs

Small and focused PRs are easier to review.

Exception: refactoring Pull Requests might logically touch several files. Make sure to comment the Pull Request accordingly.

Pull Requests And Merge Commits

Guideline Identifier: Git_PRAndMergeCommit

Once approved, commits from PR branches are squashed into a single commit prior to being merged into the main branch. The merge commit message is created from the PR title and the list of all commits in the branch linked to that PR.

To make the Git history easier to read, pay attention to polish the content of this merge commit, which essentially means polishing the PR title and your commit messages. For example, some commit messages include the full PR raw description, including Markdown comments (anything between <!-- and -->) from the GitHub PR template, which makes the Git history difficult to read once the PR is merged.

Avoid force-pushes during PR Reviews

Guideline Identifier: Git_ForcePushesDuringPullRequestReviews

Once a PR is marked for review, think of it as an ongoing conversation between you and the reviewers. Rewriting the Git history makes it harder to follow up with the review.

Unless really needed, do not force-push your branch while your Pull Request is still under review. Instead, always push new follow-up commits to your branch, so it is easier for reviewers to see and review your changes.

There are some cases where a force-push might be necessary, for example when rebasing onto the main branch to pull new changes that might be needed to fix something. In this case, make sure to let reviewers know why via a comment in the PR.

Of course, feel free to rewrite the branch history as long as there is no ongoing PR being reviewed.

Patterns

Test Tables

Use table-driven tests with subtests to avoid duplicating code when the core test logic is repetitive.

Not Recommended Good 
// func TestSplitHostPort(t *testing.T)

host, port, err := net.SplitHostPort("192.0.2.0:8000")
require.NoError(t, err)
assert.Equal(t, "192.0.2.0", host)
assert.Equal(t, "8000", port)

host, port, err = net.SplitHostPort("192.0.2.0:http")
require.NoError(t, err)
assert.Equal(t, "192.0.2.0", host)
assert.Equal(t, "http", port)

host, port, err = net.SplitHostPort(":8000")
require.NoError(t, err)
assert.Equal(t, "", host)
assert.Equal(t, "8000", port)

host, port, err = net.SplitHostPort("1:8")
require.NoError(t, err)
assert.Equal(t, "1", host)
assert.Equal(t, "8", port)
 
// func TestSplitHostPort(t *testing.T)

tests := []struct{
  give     string
  wantHost string
  wantPort string
}{
  {
    give:     "192.0.2.0:8000",
    wantHost: "192.0.2.0",
    wantPort: "8000",
  },
  {
    give:     "192.0.2.0:http",
    wantHost: "192.0.2.0",
    wantPort: "http",
  },
  {
    give:     ":8000",
    wantHost: "",
    wantPort: "8000",
  },
  {
    give:     "1:8",
    wantHost: "1",
    wantPort: "8",
  },
}

for _, tt := range tests {
  t.Run(tt.give, func(t *testing.T) {
    host, port, err := net.SplitHostPort(tt.give)
    require.NoError(t, err)
    assert.Equal(t, tt.wantHost, host)
    assert.Equal(t, tt.wantPort, port)
  })
}

Test tables make it easier to add context to error messages, reduce duplicate logic, and add new test cases.

We follow the convention that the slice of structs is referred to as tests and each test case tt. Further, we encourage explicating the input and output values for each test case with give and want prefixes.

tests := []struct{
  give     string
  wantHost string
  wantPort string
}{
  // ...
}

for _, tt := range tests {
  // ...
}

Parallel tests, like some specialized loops (for example, those that spawn goroutines or capture references as part of the loop body), must take care to explicitly assign loop variables within the loop's scope to ensure that they hold the expected values.

tests := []struct{
  give string
  // ...
}{
  // ...
}

for _, tt := range tests {
  tt := tt // for t.Parallel
  t.Run(tt.give, func(t *testing.T) {
    t.Parallel()
    // ...
  })
}

In the example above, we must declare a tt variable scoped to the loop iteration because of the use of t.Parallel() below. If we do not do that, most or all tests will receive an unexpected value for tt, or a value that changes as they're running.

Functional Options

Functional options is a pattern in which you declare an opaque Option type that records information in some internal struct. You accept a variadic number of these options and act upon the full information recorded by the options on the internal struct.

Use this pattern for optional arguments in constructors and other public APIs that you foresee needing to expand, especially if you already have three or more arguments on those functions.

Not Recommended Good 
// package db

func Open(
  addr string,
  cache bool,
  logger *zap.Logger
) (*Connection, error) {
  // ...
}
 
// package db

type Option interface {
  // ...
}

func WithCache(c bool) Option {
  // ...
}

func WithLogger(log *zap.Logger) Option {
  // ...
}

// Open creates a connection.
func Open(
  addr string,
  opts ...Option,
) (*Connection, error) {
  // ...
}

The cache and logger parameters must always be provided, even if the user wants to use the default.

db.Open(addr, db.DefaultCache, zap.NewNop())
db.Open(addr, db.DefaultCache, log)
db.Open(addr, false /* cache */, zap.NewNop())
db.Open(addr, false /* cache */, log)

Options are provided only if needed.

db.Open(addr)
db.Open(addr, db.WithLogger(log))
db.Open(addr, db.WithCache(false))
db.Open(
  addr,
  db.WithCache(false),
  db.WithLogger(log),
)

Our suggested way of implementing this pattern is with an Option interface that holds an unexported method, recording options on an unexported options struct.

type options struct {
  cache  bool
  logger *zap.Logger
}

type Option interface {
  apply(*options)
}

type cacheOption bool

func (c cacheOption) apply(opts *options) {
  opts.cache = bool(c)
}

func WithCache(c bool) Option {
  return cacheOption(c)
}

type loggerOption struct {
  Log *zap.Logger
}

func (l loggerOption) apply(opts *options) {
  opts.logger = l.Log
}

func WithLogger(log *zap.Logger) Option {
  return loggerOption{Log: log}
}

// Open creates a connection.
func Open(
  addr string,
  opts ...Option,
) (*Connection, error) {
  options := options{
    cache:  defaultCache,
    logger: zap.NewNop(),
  }

  for _, o := range opts {
    o.apply(&options)
  }

  // ...
}

Note that there's a method of implementing this pattern with closures but we believe that the pattern above provides more flexibility for authors and is easier to debug and test for users. In particular, it allows options to be compared against each other in tests and mocks, versus closures where this is impossible. Further, it lets options implement other interfaces, including fmt.Stringer which allows for user-readable string representations of the options.

See also,

Callback/Handler

This behavioral pattern can be handy for separating concerns between different layers.

A handler or callback is a function passed by the caller as an argument, either as an anonymous or a regular function. It will then get invoked at a given point in time with the appropriate arguments that are needed to continue the flow.

Not Recommended Good 
// package business

func DoSomething(s string) error {
  // The business logic here knows best
  // how to construct a Devfile object from s
  devfile := getDevfile(s)
  // Business logic should not print anything
  // here.
  // This should be the responsibility of
  // the CLI layer
  fmt.Printf("this is the Devfile: %v\n",
      devfile)
  return nil
}
 
// package business

type MyHandler func(Devfile) error

func DoSomething(s string, h MyHandler) error {
  // The business logic here knows best
  // how to construct a Devfile object from s
  devfile := getDevfile(s)
  // Do something then invoke the supplied
  // handler with the right Devfile object
  // 
  // Note that we are passing a Handler,
  // but since we know that we are just printing,
  // another valid strategy could be to
  // accept an `io.Writer` instead.
  // The handler/callback approach here
  // provides more flexibility to the callers.
  return h(devfile)
}
 
// package cli

func handleCLI() error {
  //assuming os.Args length has been validated
  business.DoSomething(os.Args[1])
  return nil
}
 
// package cli

func handleCLI() error {
  //validate os.Args length before
  business.DoSomething(os.Args[1],
    func(devfile Devfile) error {
        //This handler just needs a Devfile.
	//It does not need to worry about
        //how it is constructed.
	//It just knows that it will get called
        //at a given time with a Devfile object
        fmt.Printf("this is the Devfile: %v\n",
            devfile)
        return nil
    })
}

Enforcing coding conventions

Code Review

Refer to this document and don't hesitate to point to particular guideline identifiers. Also take a look at common principles of Go Code Review Comments. Ideally, we should provide tooling to enforce these conventions automatically.

Tooling

  • Enrich the existing validate target in Makefile
  • gosimports
  • Any other idea?

Proposing a new rule

Feel free to propose a new rule by following the process outlined below:

  • Add it to the agenda in the upcoming Cabal meetings, and let's discuss it with the rest of the team
  • If it gets accepted, add it to this guide
  • Fix all potential issues related to that new rule in the existing code
  • If possible, let's discuss a way to automatically enforce this rule, e.g., by creating/using a golangci-lint rule to detect and prevent regressions

Rules that can be enforced automatically

Note: check that automatic linters are able provide a way to purposely ignore rules if needed, e.g., via a comment in the code, a configuration file, ...

To be continued...

Clone this wiki locally