Notes from reading Practical Go

Notes of the key points copied/summarised from Dave Cheney's Practical Go for more concise reading/reference.

It's worth reading the original document as there is more exposition there of the ideas presented.

These are terse notes only; I'm not providing my own commentary, opinion or judgement on the suggestions here.

Variable naming

Longer names can indicate more importance.
Longer names may be more useful where a name is used further from its definition (e.g. far apart in same source file; in a different package)
Don't name for types.
- If the type describes the thing well, then short variable name may be appropriate
May want to match names to existing convention elsewhere.
Be consistent; use same parameter name for functions/methods.

Declaration

Be consistent in how you declare variables.
Use var for declaring, but not explicitly initialising a variable.
- var then indicates variable has been left at zero value deliberately.
- Consistent with declarations at package level
Use := when declaring and explicitly initialising.
May except rules for consistency, e.g. when you have two related variables where one has a zero value and one does not (e.g. min, max)
May also except rules and use var to indicate something more complicated is happening.
But follow local style of the project.

Comments

Document either "what", "how" or "why".

"What", explain what things available publicly do.
"How", how something works, e.g. commenting inside a function.
"Why", explain why something is the way it is, external factors for context.

Other points

Comments on variables and constants should describe contents, not purpose.
Variables and constants without initial values should describe what is responsible for initialising it.
Document public symbols. There may be exceptions for public methods that implement an interface.
Raise issues over using comments as TODOs.
Refactor code over commenting code; e.g. maybe a piece of code should be a separate function.

Packages

Naming and organisation

Changes to packages should be isolated from the rest of a codebase.
Name packages for services they provide, not what they contain.
Package names should be unique. If two packages need the same name:
- Then the name may be too generic.
- The packages overlap in functionality; merge the packages or review the design.
Avoid package names like common, util, helpers.
- "utility packages"
- They contain unrelated functions. Difficult to describe what the package provides. So, named for what it contains.
- Often appear where larger projects have deep package hierarchies and want to share code without import loops.
- Analyse where they are called and move the relevant code into the caller's package. May be better to duplicate code than introduce an import dependency.
- If you do use utility packages, prefer multiple packages, each focused on one aspect over a monolithic package.
- Example: base, common used where functionality for multiple implementations, or common types for client and server are in a single package. May be preferable to put client, server and common code in a single package, e.g. net/http does not have client and server subpackages; it has separate files client.go, server.go for respective types and transport.go for common transport code.
Note that package name is included in a qualified identifier when called by other packages, e.g. Get() in net/http is http.Get().

Return early

In Go, the style is that the successful path is down the screen as the function progresses.
Use guard clauses, conditional blocks which check conditions when entering a function, and return errors early. Makes it clear to the reader when the program flow gets to the error.

Make zero values useful

nil slices are still useful, you can append to them and don't need to make them, you can just declare them.
Example: bytes.Buffer can be written to it without initialisation.
nil pointers can still have methods called on them; those methods can check if the value is nil and provide suitable default values.

Avoid package level state

Programs should be loosely coupled; changes in one package should have a low chance of affecting another package that does not directly depend on the first.
Loose coupling in Go:
- Use interfaces to describe behaviour of functions or methods.
- Avoid global state.
Mutable global state introduces tight coupling; global variables are invisible parameter to every function in your program.
Any function that relies on a global variable can be broken if:
- the variable's type changes;
- if another of the program changes the variable.
Favour moving these variables to fields on structs.

Project structure

Projects should have a clear, single purpose.
For applications, you may have one or more main packages inside a project.
Only have public (exported) or private (not exported) access for identifiers.
Consider fewer, larger packages: more packages means more types being public.
Avoid elaborate package hierarchies; they don't have meaning in Go. You shouldn't have intermediate directories with no .go files.

Arrange code into files

Prefer internal to external tests

Internal tests, e.g. you have a package http2, then you can write http2_test.go and use package http2 inside that test file.
External tests are where you have a package identifier ending in test, e.g. package http2_test. These test files can be stored in the same place as the http2 package, but they are not part of the same package. These tests are written as if you were writing a separate package calling your code.
Example test functions for use in godoc should be in an external test file. This ensures that when viewed in godoc, the examples can work by copy-pasting.

Use `internal` packages to reduce public API surface

For projects with multiple packages, you may have exported functions for use by other packages in the same project, but not part of the public API.
Use internal in these cases: Go checks that the package that is doing the import is within the tree rooted at the parent of the internal directory, e.g. a package …/a/b/c/internal/d/e/f can only be imported by code in the directory tree rooted at …/a/b/c.

Keep `main` package small

Often assume that the things called in main.main() will only be called in that function, and only called once. This can make it hard to test.
main should parse flags, open connections to databases and loggers etc., then hand off execution to some high level object.

API Design

Design APIs that are hard to misuse

Be careful with functions with multiple parameters of the same type: some don't matter which order parameters are entered (e.g. func Max(a, b int) int) but some do func CopyFile(to, from string) error.
- Consider making a helper type that calls CopyFile, then use as from.CopyTo(to); CopyFile could be made private.

Design APIs for their default use case

Your API should not require the caller to provide parameters which they don't care about.
Discourage use of nil as a parameter, or at least don't mix nil and non nilable parameters in the same function signature.
- The use of nil can pervade through functions, if one calls another.
- API user may infer that all parameters can be nil.
Prefer variadic over []T parameters
- May have functions like func ShutdownVMs(ids []string) error that often require single arguments to be placed in a slice
- Also means you can pass empty slice or nil and it will compile, so you should cover these cases in testing.
- Instead you can use variadic parameters: but may want to have a single first parameter and a rest ... variadic parameter to ensure your function isn't called with no arguments.

Functions should define the behaviour they require

Idea of "interface segregation".
As an example, consider func Save(f *os.File, doc *Document) error
Means Save() is tied to real files, can't save to network (would require changes), can't easily test (real files), also *os.File has lots of methods irrelevant to Save().
Better to describe the parts of *os.File that are relevant: func Save(rwc io.ReadWriteCloser, doc *Document) error
Takes an interface that describes more general file-shaped things; broader in application and clarifies which methods in *os.File are relevant to the function (and hides irrelevant methods).
Can reduce to func Save(wc io.WriteCloser, doc *Document) error as unlikely that this function should read the file, if it's purpose is to save only.
But being able to close the file has two issues: it raises the question of when the wc will be closed, and it may be that the caller might want to add further data after saving this document.
Now this function is specific in terms of requirements — it needs something writable — and general in terms of function, anything that implements io.Writer interface.

Error handling

Eliminate error handling by eliminating errors

Example: using a loop controlled by bufio.NewScanner(r io.Reader) with for sc.Scan() to count lines. This returns true if the scanner matches a line of text and has encountered an error. It avoids manually checking errors in the loop to break out of the loop. It also handles io.EOF by converting it to nil unless another error is encountered; other approaches might see you have to manually check whether the error is io.EOF or not.

Example: replace an io.Writer with an errWriter that's a struct with an io.Writer and an error:

type errWriter struct {
  io.Writer
  err error
}

func (e *errWriter) Write(buf []byte) (int, error) {
  if e.err != nil {
  	return 0, e.err
  }
  var n int
  n, e.err = e.Writer.Write(buf)
  return n, nil
}

func WriteResponse(w io.Writer, st Status, headers []Header, body io.Reader) error {
  ew := &errWriter{Writer: w}
  fmt.Fprintf(ew, "HTTP/1.1 %d %s\r\n", st.Code, st.Reason)

  for _, h := range headers {
  	fmt.Fprintf(ew, "%s: %s\r\n", h.Key, h.Value)
  }

  fmt.Fprint(ew, "\r\n")
  io.Copy(ew, body)
  return ew.err
}

Now, the errors that would have to be checked after each fmt.Fprintf don't need to be; errors are stored on the errWriter, if there's already an error, no further write is attempted, and the error is returned at the end. It also enables just returning the error from errWriter, not having to pull out the error from io.Copy's return values.

Only handle an error once

Should only make one decision in response to a single error.
Example: logging an error, returning to caller that might also log it and return it, back to the top of the program. Can end up with duplicate log lines and little context at the top of the program for the error.

Adding context to errors

Ensure that you do return after an error. This may cause bugs if you don't: subsequent steps may still "succeed" without error, and return without an error even though the behaviour is not correct.
Can use fmt.Errorf to add context to an error.
Example: return fmt.Errorf("could not write config: %v", err)
This removes the possibility of logging an error, but forgetting to return it.

Wrapping errors

fmt.Errorf works for annotating the error message, but obscures the original error type. This shouldn't matter if all you are doing is checking it is not nil and then logging/printing it.
There may be less common cases where you want to recover the original error.
It mentions github.com/pkg/errors for this purpose though wrapping errors is supported in Go directly now.

Concurrency

Goroutines are often overused.
If main.main() of a Go program returns, then the program exits regardless of any other goroutines started by the program.
Example: calling http.ListenAndServe() in a goroutine called from main() compared with calling the function directly in main(). If the call is in a goroutine, you then have to stop main() from exiting.
If a goroutine can't progress until it gets the result from another, sometimes it is easier to not delegate the work.

Leave concurrency to the caller

If your function starts a goroutine, provide the caller with a way to explicitly stop that goroutine. It is often easier to leave the decision to execute functions asynchronously to the caller.

Example:

// ListDirectory returns a channel over which
// directory entries will be published. When the list
// of entries is exhausted, the channel will be closed.
func ListDirectory(dir string) chan string

The alternative might be to put all the entries into a []string and return []string, error, but could take a long time and allocate lots of memory.
This implementation is likely using a goroutine.
- Note: channels don't require goroutines to use, but they most likely do involve use of goroutines (alternative is to create a channel large enough for all the values being stored, then close the channel and return it to a caller).
Problems:
- Using a closed channel to indicate there are no more entries means that the caller cannot tell if an error occurred. It is not possible for the caller to distinguish an empty directory and a failure to read the directory.
- The caller has to continue to read from the channel until it is closed to know that the goroutine filling the channel has stopped. The caller might have received the answer they wanted before that. It might be more memory efficient than the slice implementation, especially for larger directories, but it is no quicker.
Solution: pass a function to ListDirectory that is called for each directory entry:
```
func ListDirectory(dir string, fn func(string))
```

Never start a goroutine without knowing when it will stop

Applications should be restarted from outside the application, not responsible for restarting themselves.
Only use log.Fatal() from main.main or init functions.
- If used inside functions, the program will just stop. This makes them difficult to test.
If using goroutines, then a good approach is passing errors back to the originator of the goroutine and letting the caller shut down the process cleanly.
The example given involved running http.ListenAndServe() in goroutines for application and debugging separately.

Code:

func serve(addr string, handler http.Handler, stop <-chan struct{}) error {
	s := http.Server{
		Addr:    addr,
		Handler: handler,
	}

	go func() {
		<-stop // wait for stop signal
		s.Shutdown(context.Background())
	}()

	return s.ListenAndServe()
}

func serveApp(stop <-chan struct{}) error {
	mux := http.NewServeMux()
	mux.HandleFunc("/", func(resp http.ResponseWriter, req *http.Request) {
		fmt.Fprintln(resp, "Hello, QCon!")
	})
	return serve("0.0.0.0:8080", mux, stop)
}

func serveDebug(stop <-chan struct{}) error {
	return serve("127.0.0.1:8001", http.DefaultServeMux, stop)
}

func main() {
	done := make(chan error, 2)
	stop := make(chan struct{})
	go func() {
		done <- serveDebug(stop)
	}()
	go func() {
		done <- serveApp(stop)
	}()

	var stopped bool
	for i := 0; i < cap(done); i++ {
		if err := <-done; err != nil {
			fmt.Println("error: %v", err)
		}
		if !stopped {
			stopped = true
			close(stop)
		}
	}
}

This approach has the http.ListenAndServe() calls return errors via a channel to main.main() instead of immediately calling log.Fatal().
Then in main.main(), read from this done channel reporting errors and then close a stop channel which goroutines that the servers start are receiving from. This then fills the done channel that is being used to take the return value from the goroutines; the capacity of this done channel is checked in a loop in main.main(): when filled, the program exits.

StevenMaude/practical-go.md