Go Antipatterns

These Go lang antipatterns are inspired by SQL Antipatterns. Antipattern Go code is syntactically correct, but there’s a better way that’s both functionally equivalent and idiomatic.

These are guidelines not absolutes. For example, Generic Package is common in practice and often difficult to avoid. Spending significant time trying to rename a util package can quickly amount to bikeshedding. Therefore, an Importance is listed for each antipattern:

Importance Guideline
low Easy to make better later, so it can wait
medium Becomes more difficult to change the longer it remains, so make it better as soon as possible
high Make it better right now because it reflects and effects deep technical choices that are difficult to change later

All examples are from real code but anonymized and simplified.

Writable Read-only Value

//
// Antipattern
//
func Load(&config)
//
// Better
//
func Load(config)

Importance: high

If an argument should not be modified, then don’t allow it to be modified by passing a pointer; instead, by the argument by value. This prevents accidental modification and signals to developers that the function doesn’t modify the argument. But when a function is intended to modify the argument, pass a pointer to signal this to developers.

Another way to think about this: if Load() should not modify the caller’s config, then preclude even the possibility by passing config by value. Otherwise, subtle and difficult bugs are possible when Load() mutates the config in wrong or unexpected ways causing the app to break or, worse, run like it’s ok for awhile then crash later.

Promiscuous Interface

//
// Antipattern
//
type ObjectFinder interface {
    Query(query string) []string
}
//
// Better
//
type ObjectFinder interface {
    Databases(app string) []string
    Servers(dc string) []string
}

Importance: high

Interfaces provide discrete functionality; they “specify the behavior of an object: if something can do this, then it can be used here.” (Effective Go) Methods like Query() allow the interface to do anything, making it difficult to test and mock. It also tends to leak logic: for the antipattern, the caller must know how to query for specific objects, but this logic (query syntax and semantics) is most likely not within the purview of the caller. It’s better to encapsulate this logic in the implementation of the interface. This makes the caller simpler (it doesn’t have to know query logic), and it decouples the caller from query logic which allow the logic to change without without breaking the caller.

Labyrinthine Structure

//
// Antipattern
//
type Module struct {
    *ConfigModule
}

type ConfigModule struct {
    Config              ServerConfig
    Base                *config.BaseModule
    ServiceConfigModule *config.Module
}

type ServerConfig struct {
    SSL *ServiceSSLConfig
}

type ServiceSSLConfig struct {
    KeyFile string
}

Importance: high

Structures should be shallow, preferably no more than three levels deep, especially when embedding is used. Structure and field names should be unique and descriptive. No proper example is given for the example above because it’s too conflated for clarification, probably resulting from an inherently flawed data model. To be done properly, we would need first to clarify the data model.

To highlight the antipattern, think about how KeyFile would be accessed from a Module object. It requires a nontrivial and unnecessary amount of attention to detail because of embedding and repeated use of “Config”.

Hidden Goroutine

//
// Antipattern
//
func Cleanup() {
    go func() {
        // ...
    }()
}

Cleanup()
//
// Better
//
func Cleanup() {
    // ...
}

go Cleanup()

Importance: high

All code should be necessary and sufficient. The Cleanup() function in the antipattern is not necessary. The better implementation is sufficient (and still necessary). Additionally, the antipattern breaks developer expectation by hiding the goroutine because, in general, function calls are expected to be synchronous. When a function does nothing but launch a goroutine, it is asynchronous. Functions can and should launch goroutines, but not only this. The better solution is unambiguous, explicit. It also allows Cleanup() to be called synchronously if needed, whereas the antipattern precludes this possibility (as well as testing).

New and Do

//
// Antipattern
//
func NewFoo() (*Foo, error) {
    if err := connect(); err != nil {
        return nil, err
    }
    return &Foo{}
}
//
// Better
//
func NewFoo() *Foo {
    return &Foo{}
}

func (f *Foo) Connect() error {
    if err := f.connect(); err != nil {
        return nil, err
    }
    return nil
}

Importance: high

NewFoo() in the antipattern does two things: it creates a new Foo object and it connects (to whatever; a database, for example). That functions should do one thing is a widely accepted principle (related: Single Responsibility Principle).

The antipattern also creates a tight coupling between object creation and connection, meaning the caller cannot create a Foo object and connect later, which is common when objects are not created by the caller but, for example, by a factory method. In this case, the factory should only create Foo and let the caller connect Foo when and where necessary.

The antipattern also makes it difficult to test connect() separately, and it makes the return error more difficult to handle because it could be a transient or retryable connect() error, or a fatal object creation error.

What if Foo should always be connected? What if Foo isn’t useful until connected? This is common, but it’s still better to decouple creation and connection and let the caller connect where and when necessary. It means the caller must do one more step (i.e. call Connect() after NewFoo()), but that’s a small price to pay for better, more explicit design.

There is, however, a valid reason for NewFoo() to call other functions: when the functions are necessary and sufficient to the creation of the new object. By contrast, connect() is neither necessary nor sufficient to create a Foo; it’s necessary (but perhaps not sufficient) to use a Foo. If, for example, a Foo cannot be created without the return value from bar(), then call bar() in NewFoo().

Notice also that the better example doesn’t return an error. Unless an object is complex (and if too complex, perhaps it’s doing too much and should be multiple, simpler objects), creation should be guaranteed, hence no return error. This means creation should be little more than initializing and assigning variables.

Superfluous Structure

//
// Antipattern
//
type RedisInfo map[string]string

type RawMetrics struct {
    Info RedisInfo
}

func (r *Redis) Info() RawMetrics
//
// Better
//
func (r *Redis) Info() map[string]string

Importance: medium

The simplest and most direct use of data types and structures yields code that’s easier to follow and test because there are no superfluous parts. RawMetrics, with no other fields, serves no purpose. If other fields are planned/expected (and YAGNI isn’t a stronger argument), a code comment should note it. Otherwise, it’s better to keep it simple and return the native data type: map[string]string.

Transient Argument

//
// Antipattern
//
func GetNodes(er Ranger, query string) []string {
    nodes := er.Query(query)
    // logic to transform nodes
    return nodesByName
}
//
// Better
//
func GetNodes(er Ranger) []string {
    nodes := er.GetNodes()
    // logic to transform nodes
    return nodesByName
}

Importance: medium

An argument is transient if not directly used by the function; the function only passes it to other function calls. The argument is literally only passing through the function, hence it is “transient”. The antipattern also exhibits the “Promiscuous Interface” antipattern, and the better solution uses a well-defined interface to avoid the transient argument by encapsulating the query logic in the Ranger implementation.

Runaway Arguments

//
// Antipattern
//
func NewFoo(
    id string,
    name string,
    alias string,
    userId uint,
    apiKey string,
    loc *Location,
    size int64,
    active bool,
) *Foo {
    // ...
}
//
// Better
//

// FooConfig represents required and optional values for creating a new Foo.
type FooConfig struct {
    id     string    // ID of the foo
    name   string    // Human-friendly name of the foo
    alias  string    // Alias of the foo (optional)
    userId uint      // User ID who created this foo
    apiKey string    // API key
    loc    *Location // Location of this foo
    size   int64     // Size of this foo
    active bool      // True if this foo is active
}

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

Importance: high

Functions should have at most four positional arguments. Having more is difficult for humans to remember and, if data type are the same, humans might confuse the order of arguments and cause subtle bugs (e.g. passing firstName, middleName, lastName instead of lastName, firstName, middleName). Use a struct to represent more than four arguments. This has two important benefits: the arguments can and should be documented, and it’s “future-proof”: adding new arguments is backward-compatible. The arguments struct should be immutable.

C-style Pointer

//
// Antipattern
//
var fileName *string
var files *[]FileInfo
//
// Better
//
var fileName string
var files []FileInfo

Importance: medium

It’s not typical and usually not necessary to use pointers to scalars, slices, or maps in Go. There are exceptions (see GoLang: When to use string pointers), but excessive use of pointers indicates Go being written like C (or C++ or Java) where pointers are more commonplace. In Go, the ubiquitous (and correct) use of pointers is to structures (func(t *T), return &T{}, var t = &T{}, etc.) and pointer methods (func (t *T) Foo()).

Type Hiding

//
// Antipattern
//
type Job struct { ... }
type Jobs []Job

func List(Jobs)
//
// Better
//
type Job struct { ... }

func List([]Job)

Importance: medium

Unless a custom sort on Jobs is required, Jobs should not be used because it hides the underlying data type (Job) and requires one to remember that Jobs is just []Job. Better to keep it simple and explicit: use []Job directly.

Generic Package

//
// Antipattern
//
import (
    "app/tools"
    "app/util"
    "app/misc"
    "app/db"
)

Importance: low

A package provides functionality around a logical and isolated part of the system. “What can pkg X do?” is the question we ask of that part. “How we do X” is an implementation detail that’s leaked by generic package names. Of course, naming things is difficult, and generic packages are common, but we should nonetheless attempt to clarify their purpose and place in the app with a more domain-specific package name.