Code style guidelines

Additional reading

• Elements of Programming Style (book)
  • Talk
• Effective Go
• Google Go Style Guide
• Idiomatic Go
• 20 unit test tips

Use happy path

Always adhere to returning early on error checks and negative cases. This reduces indentation and simplifies the logical flow of code.

Example

func good() (int, error) {
    // some more code
    // ...

    err := spawn()
    if err != nil {
        return 0, err
    }

    // some more code
    // ...

    return 200, nil
}

func bad() (int, error) {
    // some more code
    // ...

    err := spawn()
    if err == nil {
        // some more code
        // ...

        return 200, nil
    }

    // some more code
    // ...

    return 0, err
}

Don't use Yoda notation

Go is not C, where assignments are also expressions.

Example

// good
if err != nil {
    // ...
}

// bad
if nil != err {
    // ...
}

Don't use else and else if

Else and Else If statements are generally considered bad practice (some literature about this topic 1, 2 3 they all sort of make the same point with similar examples)

Generally almost all else and else if use cases can be refactored to not use them. Doing this usually improves readability, simplifies code, by reducing branching (usually cyclomatic complexity remains the same).

This does not mean that else and else if cases are banned, just that as a general rule avoiding them makes for cleaner code, but valid use cases do exist, like this one →

switch node := node.(type) {
// a bunch of cases, imagine 500 lines of code with complex logic, you don't
// want to touch
case *ast.Image:
    part = part.AddContainer(&jsonast.AstPart{Type: "image",}, entering)
default:
    // here is a valid use case of else, if the code want's to retain
    // the single return at the bottom, and that is the case
    // here, cause all other of the "bunch of cases~ rely on the
    // existence of the return at the bottom.
    //
    // could be refactored to not use else, but requires too much
    // work, hence "if-else" it is
    if node.AsLeaf() == nil { // container
        part = part.AddContainer(
            &jsonast.AstPart{ Type: "unknown", },
            entering,
        )
    } else {
        part.AddLeaf(
            &jsonast.AstPart{ Type: "unknown", },
        )
    }
}

return part, nil

Suggestions for how to refactor else and else if

• Use happy path.
• Most else use cases can be removed by just returning early or assigning early.
• Use switch statements.
• Code splitting.

Don't use inline/one line ifs

The use of inline ifs breaks the single purpose principle as the if statement both produces a value and checks it.

Example

// good
err := handler()
if err != nil {
    return errs.Wrap(err, "handler failed")
}

// bad
if err := handler(); err != nil {
    return errs.Wrap(err, "failed to handle")
}

Don't use %v in format strings

Where possible don't use %v, but rather exact format string directives for types. Why? - it's faster, like really noticeably faster.

Example

// good
fmt.Printf("updated in %s secs", diff.String())

// bad
fmt.Printf("updated in %v secs", diff)

Always wrap errors

Wrapping error returns provides trace of the execution flow up to the moment when something went wrong, making debugging easier, logging more verbose (if the error is logged)

Non-exhaustive list of possible exceptions to this rule are:

• recursive functions/methods
• if a package has Handle function that internal calls handle, then the internal handle call can be not wrapped to avoid duplicate messages failed to handle: failed to handle

Error returns from external package functions/methods always must be wrapped.

// good
res, err := Handler()
if err != nil {
    return errs.Wrap(err, "failed to run handler")
}

// bad
res, err := Handler()
if err != nil {
    return err
}

Add descriptive context information in error wrap message

The wrapping message must be descriptive of the failure that has occurred and where it has occurred if called from multiple locations. If the failing function operates on some data try to include some descriptors of the failing data in the error message.

Example

// good
res, err := handler(requestID)
if err != nil {
    return errs.Wrapf(err, "failed to handle request %d", requestID)
}

err := update(path)
if err != nil {
    return errs.Wrapf(err, "failed to update file %q", path)
}

Define and use error constants

If a function/method produces a new error, then instead of using errs.New (or any other error creation function) directly in the function, define a constant (technically it would be a variable) and return an error wrapping the constant. This way if the error is recoverable, callers can check the returned error against the constant.

// good
var ErrConnectionDropped = errors.New("connection dropped")

func connect(c Conn) {
    if c.IsDropped() {
        return errs.Wrapf(
            ErrConnectionDropped, "failed to connect to %q", c.String(),
        )
    }

    err := c.Connect()
    if err != nil {
        return errs.Wrap(err, "failed to connect")
    }

    return nil
}

// bad
func connect(c Conn) {
    if c.IsDropped() {
        return errs.Errorf(
            "failed to connect to %q, connection was dropped", c.String(),
        )
    }

    err := c.Connect()
    if err != nil {
        return errs.Wrap(err, "failed to connect")
    }

    return nil
}

To get errors with parameters, use error wrapping.

// bad
return errs.Errorf("invalid parameter [%s]", param)

// good
var ErrInvalidParameter = errs.New("invalid parameter")
return errs.Wrapf(ErrInvalidParameter, "parameter - %s", param)

Comment all exported functions/methods/types/variables/constants

Exported objects are the public interface of a package, having descriptive comments for them saves developers time in trying to understand what a particular piece of code does.

Write descriptive and quality comments

Try to put yourself in the shoes of a developer who's unfamiliar with the codebase trying to use the function/method in question - what information you would find useful then.

Here are some tips:

• Commented out code is strictly forbidden. It litters the codebase with lines that no-one will likely touch.
• Good comments shorten the "understanding" process of someone else reading the code.
• Explain the decisions made "why is something written this way and not the other"
• Explain the limitations of a certain piece of code.
• Writing modular code with nesting structs each handling some part of the applications' logic, write one large comment for the struct, explaining its purpose. Then further comments on functions/methods can be less verbose, generally function/method name should be enough to explain what it does (if it's not, then it's likely a sign that the function/methods scope is to large)

Always add type checks for a type that implements some interface

When a type implements an interface, add a type check with the type against the implemented interface. This provides a compile time check that the type actually implements the desired interface.

This also provides a convenient type check that the compiler will start screaming about if you change the implementations or interfaces signature.

Example

var (
    _ io.Writer = (*customBuffer)(nil)
    _ io.Reader = (*customBuffer)(nil)
)

type customBuffer struct {}

Don't use interface{} and any types

Strict types are Go's power. Using them allows catching errors, mistakes, typos quickly and at compile time. Use of exact, strict types is one of Go's idioms.

Don't use interface{} type, use any

Even though use of any/interface{} types is discouraged by previous rule, there are valid usages and usages that can't be avoided. In such cases use any instead of interface{} type. interface{} is deprecated. any is just a type alias to interface{}, it provides a clearer separation of interface{}/any - the type and interface - a set of method signatures that a type can implement.

Use range when iterating slices, accessing slice elements

Go has a range expression, for convenience and to reduce common out-of-bounds errors. Use range.

Example

// good
for idx, element := range slice{
    // ...
}
// also good
for _, element := range slice {
    // if you don't need the index.
    // ...
}

// bad
for idx := 0; idx < len(slice); idx++ {
    // ...
}

Don't use named returns

In short →

func handle() (error) { return nil } // good
func handle() (err error) { return } // bad

Named returns have two main use cases:

1. documenting the return types (giving them a name)
2. catching return value in a defer

Automatic documentation sounds really nice at first, but falls apart quickly when analyzed deeper. The thing is that only thing pretty from the use of named returns is the signature. More importantly the function/methods body becomes a mess. Data flows get obfuscated, additional variable declarations are required.

Scenario 1.

func handle() (status int, err error) {
    // big messy spaghetti function
    // ...
    // ...

    if (cond) {
        return
    }

    // big messy spaghetti function continues
    // ...
    // ...

    return
}

• Q: What is the function returning?
• A: Not enough context you can know unless you understand the function completely. So every time you want to find the source of some data-flow you would have to understand each method/function that uses named returns completely.

On the other hand consider the following function

func handle() (int, error) {
    // big messy spaghetti function
    // ...
    // ...

    if (cond) {
        return 0, err
    }

    // big messy spaghetti function continues
    // ...
    // ...

    return 200, nil
}

• Q: What is the function returning?
• A: Now you can immediately tell what are the return values. Without even reading the spaghetti code.

Catching return values with defer in methods/functions with named returns is a valid use case of named returns. The only problem is that if you are in a situation where you need to do that you are likely doing something else wrong. That something else is likely single purpose methods/functions.

Function that does one thing only can be rewritten as:

func whapHandle() error {
    err := handle()
    if err != nil {
        return zbxerr.Wrap(err, "handle failed")
    }

    return nil
}

from:

func wrapHandle() (err error) {
    defer func() {
        if err != nil {
            err = zbxerr.Wrap(err, "handle failed")
        }
    }()

    return handle()
}

Doing some complex logic in a defer needing to use a named return is a sign that you are trying to fit in too much logic into a single scope. Split it up.

Split long lines to abide by line length limits

Almost all long lines can be split to abide by line length limits. Split long lines, makes the codebase consistent and easy to read for other developers.

Examples - how to split long lines

Function / Method Calls

// function/method invocations
handler(long, many, arguments) // exceeds line length limit
// split
handler(
    long, many, arguments,
)
// if still exceeds line length limit (though at this point you should
// reconsider the logical structure of your code, as functions with this many
// arguments might be handling too many things in a single scope)
handler(
    long,
    many,
    arguments,
)

// function/method definitions
func (h *handler) handle(long, many, params) (long, many, returns) {} // exceeds line length limit
// split
func (h *handler) handle(
    long, many, params,
) (long, many, returns) {}
// if still exceeds line length limit
func (h *handler) handle(
    long,
    many,
    params,
) (long, many, returns) {}
// if still, still exceeds line length limit (consider restructuring logic)
func (h *handler) handle(
    long,
    many,
    params,
) (
    long,
    many,
    returns,
) {}

Don't define global variables

Don't define global variables exported or un-exported. Packages that define, use or rely on global variables are unsalable, not modular and harder to maintain.

Don't use module init functions

Where possible don't use init functions for modules. The use of init functions means that there are global variables that need initialization (see rule about global variables, that explains why you shouldn't use them).

Example

// good
package handlers

func NewHandler() *Handler {
    return &Handler{
        // ...
    }
}

// in a different package
package server

func StartServer() {
    h := handlers.NewHandler()
    h.Serve()
}

// bad
var H *Handler

func init() {
    H = &Handler{
        // ...
    }
}

Order objects (constants, variables, functions/methods) of a file in a consistent order.

.go files must have the following structure:

• constants block
  • exported constants
  • un-exported constants
• variables block
  • exported variables
  • un-exported variables
• types
  • exported types
  • un-exported types
• init function (only when refactoring existing code)
• exported functions/methods
• un-exported functions/methods

Lint should catch any deviations of the structure.

Use io.Stdout to write to stdout

Functions like fmt.Printf, fmt.Println are commonly used for debugging purposes. Use fmt.Fprint to explicitly state that you want to write to io.Stdout and that you haven't just forgotten to remove a debug print.

Go unit test style guidelines

Generate unit test boilerplate with gotests

gotests tool is a CLI tool that generates table driven test boilerplate skeletons from predefined generation templates. Using this tool ensures that unit test base structure is consistent for every one, makes tests easier to write, saves time.

Install gotests with:

go install github.com/cweill/gotests/gotests@latest

Download predefined unit test templates for Zabbix - templates.

How are Zabbix templates different from the default ones?

• Tests use cmp.Diff from https://github.com/google/go-cmp to compare complex objects. Its faster and provides better diffs that reflect.DeepEqual.
• Tests and test cases run in parallel by default using t.Parallel(). If a test or its cases can't be run in parallel, then the t.Parallel() calls can be removed, but by default encourage parallel test execution.
• Tests use t.Fatalf() to failing cases.

To generate a test from the command line run

gotests \
  -template_dir '/path/to/zabbix/test/templates/dir' \
  -only 'nameOfFuncionToTest' \
  -w \
  '/path/to/source/code/file/or/dir'

Flags used:

• -template_dir custom unit test skeleton templates for Zabbix (read more about the custom templates below)
• -only the function or methods name to test
• -w write the generated skeleton to _test.go file.

Check https://github.com/cweill/gotests for integration of your choice of editor. Don't forget to configure use of Zabbix templates.

Test case names must have a prefix + for positive cases or - for negative cases.

A positive case is when the function being tested produces a valid result. A negative case is when the function produces invalid result or is called with invalid input.

Example

{
    "+valid",
    // ...
},
{
    "-emptyInput",
    // ...
},
{
    "-updateErr",
    // ...
},

The first case name should be +valid

The +valid case demonstrates the successful flow of the function being tested.

{
    "+valid",
    // ...
}

Test case names must camelCase, first letter lower case

Example

{
    "-requestUpdateErr",
    // ...
}

Order cases to follow the execution flow of the test target function.

The cases of a test must be ordered in a way that aligns with the execution flow of the function/method, placing positive cases first, followed by all negative cases.

Example

The test target function:

func (h *handler) proxyFile(w http.ResponseWriter, path string) error {
    req, err := http.NewRequestWithContext(
        context.Background(),
        http.MethodGet,
        path,
        nil,
    )
    if err != nil {
        return errors.Wrap(err, "failed to create request")
    }

    resp, err := h.httpClient.Do(req)
    if err != nil {
        return errors.Wrap(err, "failed to do request")
    }

    defer resp.Body.Close()

    _, err = io.Copy(w, resp.Body)
    if err != nil {
        return errors.Wrap(err, "failed to copy source request response")
    }

    w.WriteHeader(resp.StatusCode)

    return nil
}

The test cases:

• +valid
• -emptyPath
• -errCreatingNewRequest
• -errDoingRequest
• -errCopyingResp

Use t.Fatalf to fail a case

Use cmp.Diff to produce human-readable diffs

Example

ret, err := function()
// ... other checks ...
if diff := cmp.Diff(tt.wantRet, ret); diff != "" {
    t.Fatalf("function() return value mismatch (-want +got):\n%s", diff)
}

Note that an inline if statement is used here to mimic a simple if x != y check and is the default in unit test generation template.

Make case failure messages easy to read and understand

Use cmp.Diff to compare anything more complex than a int, and print the diff, describing exactly what mismatched.

Describe in failure messages, exactly what went wrong, what was the expected result.

Failure messages must have the prefix receiver.method() or function()

Having the prefix makes it easier to understand where a test failure is coming from.

Example

t.Fatalf(
    "ConnCollection.newConn() error = %v, wantErr %v",
    err, tt.wantErr,
)

Here ConnCollection is the receiver and newConn is the method.

Cases testing code with loops must trigger them at least twice

This way the case checks that the loop actually does what it's intended to.

Example

Unit test target function:

func prefix(p string, words []string) []string {
    ret := make([]string, 0, len(s))

    for _, w := range words {
        ret = append(ret, p + w)
    }

    return ret
}

Test case for prefix must contain a case that make the loop iterate twice

{
    "+valid",
    args{
        "prefix",
        // more that two element == more than two iterations
        []string{"a", "b", "c"},
    },
    []string{"prefixa", "prefixb", "prefixc"},
}

Bad example case:

{
    "+badDontDoThis",
    args{
        "prefix",
        // triggers the loop in the tested function only once. for all we know
        // from this test all that the function might be doing is
        // return []string{"prefixa"}
        []string{"a"},
    },
    []string{"prefixa"},
}

Don't share data between tests

It's common that unit test input/output data can be shared between tests. If tests share data, then small changes to code have a cascading effect in tests, where changing one part of shared input/output data requires adjusting all tests that use it.

Define input and output data in each test scope.

Use expect field for cases when using mocks.

When using mocks add a field to the cases' table expect. It should be a struct with boolean values for each mock expectation that the function has.

This way each case controls what expectation on the function should be placed.

Example

type expect struct {
    read   bool
    update bool
}

tests := []struct {
    name           string
    expect         expect
    fields         fields
    args           args
    wantErr        bool
}{
    // ...
}
for _, tt := range tests {
    tt := tt
    t.Run(tt.name, func(t *testing.T) {
        t.Parallel()

        mock := &Mock{}

        if tt.expect.read {
            mock.ExpectRead()
        }

        if tt.expect.update {
            mock.ExpectUpdate()
        }

        err := function(mock)
        if (err != nil) != tt.wantErr {
            t.Fatalf("function() error = %v, wantErr %v", err, tt.wantErr)
        }
        if err := mock.ExpectationsWhereMet(): err != nil {
            t.Fatalf(
                "function() DB mock expectations where not met: %s",
                err.Error(),
            )
        }
    })
}

Plugin packages

Helper

Helper packages help in working of and keeping Zabbix agent 2 Go functionality consistent. The most commonly used packages are zbxerr, log and uri. The full list of packages utilized by Zabbix is given in the Overview section.

Zbxerr

Zbxerr package handles error formatting. The instructions on how to create a new error message are given below:

1. To create a new error message:

zbxerr.New(err.Error())

2. To create a custom error message:

zbxerr.New("custom error").Wrap(err)

3. To minimize error text inconsistencies, it is recommended to use predefined error messages, already available by default in zbxerr package.

ErrorTooManyParameters
ErrorInvalidParams
ErrorTooFewParameters
ErrorTooManyParameters

4. To create a more detailed error message, the cause of the error can be added at the end of the predefined error message:

zbxerr.ErrorCannotMarshalJSON.Wrap(err)

go get example.com/pkg

go get example.com/pkg@v1.2.3

go get

go mod tidy

    var testFlag string
    const (
        testDefault     = ""
        testDescription = "description"
    )
    flag.StringVar(&testFlag, "test", testDefault, testDescription)
    flag.StringVar(&testFlag, "t", testDefault, testDescription+" (shorthand)")

func loadOSDependentFlags() {}

Formatting

Follow the instructions given below.

1. Go code must conform to the rules specified in a configuration file:

src/go/.golangci.yml

2. Thus, the following line:

 make style NEW_FROM_REV=HEAD~X

3. Must not produce warnings, for example, this one:

make style NEW_FROM_REV=HEAD~2