Skip to main content

Golang Programming Guide

Style and Conventions​

Generally speaking, we will follow the recommendations laid out in Go Code Review Comments. By its own admission, this page:

...collects common comments made during reviews of Go code, so that a single detailed explanation can be referred to by shorthands. This is a laundry list of common mistakes, not a style guide.

Despite not being an official style guide, it covers a good amount of scope in a concise format, and should be able to keep our project code fairly consistent.

Beyond what is described above, the following contain additional insights into how to write better Go code.

Acronyms​

Domain concepts should be used without abbreviation when used alone.

DoAvoid
TransportationServiceProviderTSP
TrafficDistributionListTDL

However, when used as a specifier or part of another name, names that have existing acronyms should use the acronym for brevity.

DoAvoid
TSPPerformanceTransportationServiceProviderPerformance

Acronyms should always be either all caps or all lower-cased.

DoAvoid
TSPPerformancetSPPerformance
tspPerformancetspperformance
TspPerformance

Importing Dependencies​

Dependencies are managed by dep. New dependencies are automatically detected in import statements. To add a new dependency to the project:

  1. Add the package to the import statement of a Go file.
  2. make clean
  3. make server_generate
  4. dep check (to verify what's missing.) If it looks reasonable then...
  5. dep ensure

Querying the Database Safely​

  • SQL statements must use PostgreSQL-native parameter replacement format (e.g. $1, $2, etc.) and never interpolate values into SQL fragments in any other way.
  • SQL statements must only be defined in the models package.

Here is an example of a safe query for a single Shipment:

// db is a *pop.Connection

id := "0186ad95-14ed-4c9b-9f62-d5bd124f62a1"

query := db.Where("id = $1", id)

shipment := &models.Shipment{}
if err = query.First(shipment); err == nil {
  pp.Println(shipment)
}

models.Fetch* functions​

Functions that return model structs should return pointers.

Do

func FetchShipment(db *pop.Connection, id uuid.UUID) (*Shipment, error) {}

Avoid

func FetchShipment(db *pop.Connection, id uuid.UUID) (Shipment, error) {}

This is for a few reasons:

  • Many Pop methods that accept models need to use pointers in order to modify the struct being passed in. Using them everywhere models are used makes the code more consistent and avoids needing to convert to and from pointers.
  • Any methods on struct model instances need to have pointer receivers in order to mutate the struct. This is a common point of confusion and an easy way to introduce bugs into a codebase.

Logging​

We use the Zap logging framework from Uber to produce structured log records. To this end, code should avoid using the SugaredLoggers without a very explicit reason which should be record in an inline comment where the SugaredLogger is constructed.

Logging Levels​

Another reason to use the Zap logging package is that it provides more nuanced logging levels than the basic Go logging package. That said, leveled logging is only meaningful if there is a common pattern in the usage of each logging level. To that end, the following indicates when each level of Logging should be used.

  • Fatal This should never be used outside of the server startup code in main.go. Fatal log messages call sys.exit(1) which unceremoniously kills the server without running any clean up code. This is almost certainly never what you want in production.

  • Error Reserved for system failures, e.g. cannot reach a database, a DB insert which was expected to work failed, or an "enum" has an unexpected value. In production an Error logging message should alert the team that all is not well with the server, so avoid being the 'Boy Who Cried Wolf'. In particular, if there is an API which takes an object ID as part of the URL, then passing a bad value in should NOT log an Error message. It should log Info and then return 404.

  • Warn Don't use Warn - it rarely, if ever, adds any meaningful signal to the logs.

  • Info Use for recording 'Normal' events at a granularity that may be helpful to tracing and debugging requests, e.g. 404's from requests with bad IDs, authentication events (user logs in/out), authorization failures etc.

  • Debug Debug events are of questionable value and should be used during development, but probably best removed before landing changes. The issue with them is, if they are left in the code, they quickly become so dense in the logs as to obscure other debug log entries. This leads to people an arms race of folks adding 'XXXXXXX' to comments in order to identify their log items. If you must use them, I suggest adding an, e.g. zap.String("owner", "nick")

Use JSON friendly names for Zap.Objects

Use JSON friendly names for zap.Object so that you will be able to extract the values easily in CloudWatch Insights queries.

s.logger.Info("EDIs processed", zap.Object("edisProcessed", &ediProcessing))

vs

s.logger.Info("EDIs processed", zap.Object("EDIs Processed", &ediProcessing))

The latter was nearly impossible to filter on. But the following insights query works

fields @timestamp, edisProcessed, @message
| sort @timestamp desc

Errors​

Some general guidelines for errors:

1. Don't bury your errors in underscores.​

If a function or other action generates an error, assign it to a variable and either return it as part of your function's output or handle it in place (if err != nil, etc.). There will be the very occasional exception to this - one is within tests, depending on the test's goal. If you find yourself typing that underscore, take a moment to ask yourself why you're choosing that option. On those very rare occasions when it is the correct behavior, please add a comment explaining why.

Don't:

myVal, _ := functionThatShouldReturnAnInt()

Do:

myVal, err := functionThatShouldReturnAnInt()
if err != nil {
     return myVal, fmt.Errorf("function didn't return an int: %w", err)
}

2. Log at the top level; create and pass along errors below.​

If you're creating a query (1) that is called by a function (2) that is in turned called by another function (3), create and return errors at levels 1 and 2 (and possibly handle them immediately after creation, if needed), and log them at level 3. Logs should be created at the top level and contain context about what created them. This is more difficult if logs are being created in every function and file that supports the operation you're working on. Here's an example of when to create errors and when to handle them:

In pkg/models/blackout_dates.go, an error is created and returned:

func FetchTSPBlackoutDates(tx *pop.Connection, tspID uuid.UUID, shipment Shipment) ([]BlackoutDate, error) {
  ...
  err = query.All(&blackoutDates)
  if err != nil {
    return blackoutDates, fmt.Errorf("Blackout dates query failed: %w", err)
  }

  return blackoutDates, err
}

In pkg/awardqueue/awardqueue.go, FetchTSPBlackoutDates is called, and any possible error is handled. This function also returns an error.

func ShipmentWithinBlackoutDates(tspID uuid.UUID, shipment models.Shipment) (bool, error) {
  blackoutDates, err := models.FetchTSPBlackoutDates(aq.db, tspID, shipment)

  if err != nil {
    return false, fmt.Errorf("Error retrieving blackout dates from database: %w", err)
  }

  return len(blackoutDates) != 0, nil
}

Finally, at the top level in attemptShipmentOffer in the same file, any errors bubbled up from ShipmentWithinBlackoutDates or FetchTSPBlackoutDates are handled definitively, halting the progress of the longer function if the underlying processes and queries didn't complete as expected in the functions being called:

func (aq *AwardQueue) attemptShipmentOffer(shipment models.Shipment) (*models.ShipmentOffer, error) {
  aq.logger.Info("Attempting to offer shipment", zap.Any("shipment_id", shipment.ID))
  ...
  isAdministrativeShipment, err := aq.ShipmentWithinBlackoutDates(tsp.ID, shipment)
  if err != nil {
    aq.logger.Error("Failed to determine if shipment is within TSP blackout dates", zap.Error(err))
    return err
  }
  ...
}

The error is created and passed along at the lowest level, logged and passed along at the middle level (along with other errors that can happen within that function), and logged again at the highest level before finally halting the progress of the process if an error is present.

3. Use fmt.Errorf instead of errors.New​

You should avoid using errors.New when creating an error in go. Please use fmt.Errorf instead.

Current Recommendation:

return fmt.Errorf("Some application error message")

Old Recommendation:

return errors.New("Some application error message")

4. Use fmt.Errorf and %w verb when wrapping errors.​

fmt.Errorf() with %w provides greater error context and a stack trace, making it especially useful when dealing with the opacity that sometimes comes with external libraries. As of go 1.13 fmt.Errorf() provides a new %w verb that replaces the need for errors.Wrap(). It takes a string with the %w in it and the error as parameters: the string and error to provide context and explanation. Keep the string brief and clear, assuming that the fuller cause will be provided by the context %w verb brings. It can also add useful context for errors related to internal code if there might otherwise be unhelpful opacity. fmt.Errorf() in combination with %w also capture stack traces with the additional function of string substitution/formatting for output.

Instead of just returning the error, offer greater context with something like this:

Current Recommendation:

if err != nil {
    return fmt.Errorf("Pop validate failed: %w", err)
}

As this is a recent change in golang standards you will see many uses of the errors.Wrap still in the code, which had been the old recommendation. It is safe to change code that wraps errors as below to the new standard in the example above.

Old Recommendation:

if err != nil {
    return errors.Wrap(err, "Pop validate failed")
}

Using the new %w provides access to some new features such as the new errors.Is and errors.As functions. For more on the changes in go 1.13 see this official blog post and this article that go into more depth.

5. Don't fmt errors; log instead.​

fmt can provide useful error handling during initial debugging, but we strongly suggest logging instead, from when you write the initial lines of a new function. Using logging creates structured logs instead of the unstructured, human-friendly-only output that fmt does. If an fmt statement offers usefulness beyond your initial troubleshooting while working, switch it to fmt.Errorf("text: %w", err) or logger.Error(), perhaps with Zap.

Don't:

fmt.Println("Blackout dates fetch failed: ", err)

Do:

logger.Error("Blackout dates fetch failed: ", err)

6. If some of your errors are predictable, pattern match on them to provide more error detail.​

Some errors are predictable, such as those from the database that Pop returns to us. This gives you the option to use those predictable errors to give yourself and fellow maintainers of code more detail than you might get otherwise, like so:

// FetchServiceMemberForUser returns a service member only if it is allowed for the given user to access that service member.
 func FetchServiceMemberForUser(ctx context.Context, db *pop.Connection, user User, id uuid.UUID) (ServiceMember, error) {
  var serviceMember ServiceMember
  err := db.Eager().Find(&serviceMember, id)
  if err != nil {
    if errors.Cause(err).Error() == recordNotFoundErrorString {
      return ServiceMember{}, ErrFetchNotFound
    }
    // Otherwise, it's an unexpected err so we return that.
    return ServiceMember{}, err
  }

  if serviceMember.UserID != user.ID {
    return ServiceMember{}, ErrFetchForbidden
  }

  return serviceMember, nil
 }

You can also use fmt.Errorf("text: %w", err) in this situation to provide access to even more information, beyond the breadcrumbs left here.

Libraries​

Pop​

We use Pop as the ORM(-ish) to mediate access to the database. The Unofficial Pop Book is a valuable companion to Pop’s GitHub documentation.

Learning​

If you are new to Go, you should work your way through all of these resources (in this order, ideally):

  1. A Tour of Go (in-browser interactive language tutorial)
  2. How to Write Go Code (info about the Go environment, testing, etc.)
  3. Effective Go (how to do things “the Go way”)
  4. Daily Dep documentation (common tasks you’ll encounter with our dependency manager)
  5. Exercism offers a series of exercises with gradually increasing complexity

Additional resources:

Service Objects​

Service Objects are an architectural design pattern we use to encapsulate business logic. Service objects allow for better unit testing, re-usability, and organization of code in the MilMove project. For more information on service objects, including how we use them and how to write them, please see the documentation.

Testing​

Knowing what deserves a test and what doesn’t can be tricky, especially early on when a project’s testing conventions haven’t been established. Use the following guidelines to determine if and how some code should be tested.

General​

  • Use table-driven tests where appropriate.
  • Make judicious use of helper functions so that the intent of a test is not lost in a sea of error checking and boilerplate. Use t.Helper() in your test helper functions to keep stack traces clean.

Coverage​

  • Always test exported functions. Exported functions should be treated as an API layer for other packages. Cover the expected behavior and error scenarios as a user of that API.
  • Try not to test unexported functions. Unexported functions are implementation details of exported ones and should not change the intended usage. If you find that an unexported function is complex and needs testing, it might mean it needs to be refactored as it's exported function elsewhere.

Models​

In general, focus on testing non-trivial behavior.

  • Structs do not need to be tested as they have no behavior of their own.
  • Struct methods warrant a unit test if they contain important behavior, e.g. validations.
  • Avoid testing functionality of libraries, e.g. model saving and loading (which is provided by Pop)
  • Try to leverage the type system to ensure that components are “hooked up correctly” instead of writing integration tests.

Time​

Some helpful tips on dealing with time in the MilMove Go codebase can be found in this doc

Miscellaneous Tips​

  • Use golang instead of go in Google searches.
  • Try to use the standard lib as much as possible, especially when learning.

Environment settings​

Adding ulimit​

Dep appears to open many files simultaneously, particularly as the project matures and depends on more and more third-party repositories. You may encounter a message like this as a dep-status hook error when trying to commit locally: remote repository does not exist, or is inaccessible: : pipe: too many open files.

To fix this, run ulimit -n 5000 in your terminal. This increases the number of file handles (the details on files that a process holds open) on your system. You can run ulimit -n to see how many are currently allowed; you may see a number like 128 or 256. (Run ulimit -a to see all current limits on your system, including pipe size, stack size, and user processes.) If running this in your terminal window allows you to complete your commit, you may wish to add it to /.bash_profile or whichever system file you use for your terminal settings.