else
and else if
%v
in format stringsrange
when iterating slices, accessing slice elementsinit
functionsio.Stdout
to write to stdoutgotests
+
for positive cases or -
for negative cases.+valid
t.Fatalf
to fail a casecmp.Diff
to produce human-readable diffsreceiver.method()
or function()
expect
field for cases when using mocks.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
}
Go is not C, where assignments are also expressions.
Example
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
else
and else if
else
use cases can be removed by just returning early or assigning early.switch
statements.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")
}
%v
in format stringsWhere 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)
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:
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
}
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)
}
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
}
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.
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:
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 {}
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.
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.
range
when iterating slices, accessing slice elementsGo 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++ {
// ...
}
In short →
Named returns have two main use cases:
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
}
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
}
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.
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 exported or un-exported. Packages that define, use or rely on global variables are unsalable, not modular and harder to maintain.
init
functionsWhere 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{
// ...
}
}
.go
files must have the following structure:
Lint should catch any deviations of the structure.
io.Stdout
to write to stdoutFunctions 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.
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:
Download predefined unit test templates for Zabbix - templates.
How are Zabbix templates different from the default ones?
cmp.Diff
from https://github.com/google/go-cmp to compare complex objects. Its faster and provides better diffs that reflect.DeepEqual
.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.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 bellow)-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.
+
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
The +valid
case demonstrates the successful flow of the function being tested.
Example
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
t.Fatalf
to fail a casecmp.Diff
to produce human-readable diffsExample
ret, err := function()
// ... other checks ...
if diff := cmp.Diff(tt.wantRet, ret); diff != "" {
t.Fatalf("function() return value mismatch (-want +got):\n%s", diff)
}
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.
receiver.method()
or function()
Having the prefix makes it easier to understand where a test failure is coming from.
Example
Here ConnCollection
is the receiver and newConn
is the method.
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"},
}
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.
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(),
)
}
})
}
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 package handles error formatting. The instructions on how to create a new error message are given below:
::: noteclassic See the samples for Error messages and Zabbix naming guidelines :::
When it is necessary to add third-party libraries, it is done in a following way:
go get
command from ~/zabbix/src/go
directory:Any additional plugin parameter configuration is also done in plugins. See the instructions below.
go.mod
file:go.sum
, it might be necessary to run this one:Parameters for Zabbix agent 2 are configured utilizing the native Go flag
package. See the instructions for configuration below.
var testFlag string
const (
testDefault = ""
testDescription = "description"
)
flag.StringVar(&testFlag, "test", testDefault, testDescription)
flag.StringVar(&testFlag, "t", testDefault, testDescription+" (shorthand)")
::: noteclassic Parameters for Zabbix agent 2 plugins are defined and handled separately for each plugin. Typically, they are checked for the allowed number of parameters and values. :::
Follow the instructions given below.
Go code
must conform to the rules specified in a configuration file:::: noteimportant In this syntax, X denotes the oldest commit number from where the changes began. :::
::: noteclassic In order to run the checks, it is required to install golangci-lint:. :::