Switch branches/tags
Nothing to show
Find file History
Permalink
..
Failed to load latest commit information.
README.md Code review: closing request body after err check Jun 21, 2018

README.md

golang code review

This is an informal, WIP list of the things things I (@gonzaloserrano) do when reviewing Golang code.

general refs

code reviewing & github

  • the guidelines for faster PR reviews from the Kubernetes project are a must. A quick summary:
    • do small commits and, even better, small PRs
    • use separate PRs for fixes not related with your feature
    • add a different commit for review changes so it's easy to review them instead of the whole patch
    • test and document your code
    • don't add features you don't need
  • other guidelines:
    • prefer documentation as code (example tests files) over READMEs
    • separate the vendor updates in a different commit
    • choose a good GitHub merge strategy:
      • choose merge when:
        • multiple commits
        • deps in different commit
      • choose squash if you just have a single commit to avoid the extra merge commit
    • do Continuous Integration (CI) to ensure the code quality:
      • tests are in green (go test -race in TravisCI or CircleCI)
      • new code or changes in functionality has the corresponding tests (e.g gocovmerge + codecov.io or coveralls.io)

code linting

The code should pass a linter step in the CI pipeline:

mandatory:

others:

  • errcheck with -blank: check your errors are handled properly
  • interfacer: use the narrowest interface available.
  • gosimple: simplify your code whenever possible.
  • statticcheck: advanced tool to find subtle improvements and bugs in your code.

gonmetalinter: a dummy shell script I did for launching some linters

software design

package design

  • learn and follow the stdlib design
  • packagitis: this is not PHP or Java, where you put classes wherever and then you refer to them with the namespace.
  • watch out import cycles! Go makes you think at compiler time how your types will be grouped.
  • are the exported types needed to be exported?
    • maybe they are just used for tests
  • reduce the number of package dependencies
  • Bill Kennedy's resources:
  • style guides for go pkgs - @rakyll:
    • use multiple files
    • keep types closer to where they are used
    • organize by responsibility (e.g avoid package model)

errors

concurrency

  • read golang concurrency tricks
  • go makes concurrency easy enough to be dangerous source
  • shared mutable state is the root of all evil source
  • how protect shared mutable data
    • with Mutex/RWMutex
    • or avoid sharing state and use message passing via channels
    • or use the atomic pkg if your state is a primitive (float64 for e.g)
      • that's a nice thing for testing using spies in concurrent tests for e.g
    • or use things like sync.Once or sync.Map (>= go 1.9)
  • testing:
    • good concurrent tests are mandatory and run with -race flag
    • use parallel subtests when possible to make tests run faster (official blog post)
      • must capture range vars! tc := tc // capture range variable (also see next point)
  • watch out when:
    • launching goroutines inside loops, see using goroutines on loop iteration variables
    • passing pointers through channels
    • you see the go keyword around without much explanation and tests
      • why was it done?
      • is there a benchmark for that?
    • async fire-and-forget logic: does that function return or not a value and/or error that must be handled?
  • goroutine lifetime:
  • if you write libs, leave concurrency to the consumer of the lib when possible
    • your code will be simpler, and the clients will choose the kind of concurrency they want
  • don't expose channels
  • the channel closing principle: don't close a channel from the receiver side and don't close a channel if the channel has multiple concurrent senders.
  • refs:
  • performance:

http

naming

Be careful when using short names. Usually there is no prob with receivers names, or temporal variables like indexes in loops etc. But if your funcs violate the SRP or you have many args, then you can end with many short names and it can make more harm than good. If understanding a var name is not straightforward, use longer names.

other refs:

gotchas

  • 50 Shades of Go
  • a time.Ticker must be stopped, otherwise a goroutine is leaked
  • slices hold a referente to the underlying array as explained here and here. Slices are passed by reference, maybe you think you are returning small data but it's underlying array can be huge.
  • interfaces and nil gotcha, see the understanding nil talk by Francesc Campoy

tests

  • test names & scope
  • coverage
    • edge cases
    • any change of behaviour should have a test that covers it
    • i.e new code yes, a refactor maybe not
  • scope: if tests are complicated think about
    • refactor the code
    • refactor the tests
    • add arrange / act / assert comments
  • types:
    • normal
    • internal
    • integration
    • example files
    • benchmarks
  • subtests: see concurrency.
  • tags: if you have tagged some tests (e.g // +build integration) then you need to run your tests with -tags
  • t.Run and t.Parallel
    • watch out with test cases in a loop, you need probably something like tc := tc before
  • assert should have (expected, actual) not on the contrary
  • test doubles naming (from the little mocker by Uncle Bob):
    • dummy objects are passed around but never actually used
    • fake objects actually have working implementations, but usually take some shortcut which makes them not suitable for production (an InMemoryTestDatabase is a good example).
    • stubs provide canned answers to calls made during the test, usually not responding at all to anything outside what's programmed in for the test.
    • spies are stubs that also record some information based on how they were called. One form of this might be an email service that records how many messages it was sent.
    • mocks are pre-programmed with expectations which form a specification of the calls they are expected to receive. They can throw an exception if they receive a call they don't expect and are checked during verification to ensure they got all the calls they were expecting.

aws-sdk-go

  • Create just a single session in the top level, see the doc
  • Every service package has an interface that you can use for embedding in test, see an example in the official blog

performance