Part of https://discourse.jujucharms.com/t/read-before-contributing/47, an opinionated guide by William Reade
Interfaces Are Awesome
They really, really are – they create an environment in which creating (SO)LID types is the path of least resistance. But while they are an exceptionally useful tool, they are not the right tool for every job, and injudiciously chosen interfaces can hamper understanding as much as good ones can aid it.
Awesome Interfaces Are Small
One method is a great size for an interface; 3 or 4 is probably
perfectly decent; more than that and you’re probably creating an interface to cover up for some bloated concrete type that you’re replacing so you can test the recipient.
And that’s tolerable – it’s certainly progress – but it’s also quite possibly a missed opportunity. Imagine a config struct that currently references some horrible gnarly type:
type Config struct {
State *state.State
Magic string
}
…and an implementation that uses a bunch of methods, such that you extract the following interface to give (note: not real State methods, methods returning concrete types with unexported fields are a goddamn nightmare for testing and refactoring, this is heavily idealized for convenience):
type Backend interface {
ListUnits() ([]string, error)
GetUnits([]string) ([]Unit, error)
DestroyUnits([]string) error
ListMachines() ([]string, error)
GetMachines([]string) ([]Machine, error)
DestroyMachines([]string) error
}
type Config struct {
Backend Backend
Magic string
}
This is great, because you can now test your implementation thoroughly by means of a mock implementing the Backend interface, but it’s actually still pretty unwieldy.
If we go a step further, though, according to the capabilities that are most intimately connected:
type UnitBackend interface {
ListUnits() ([]string, error)
GetUnits([]string) ([]Unit, error)
DestroyUnits([]string) error
}
type MachineBackend interface {
ListMachines() ([]string, error)
GetMachines([]string) ([]Machine, error)
DestroyMachines([]string) error
}
type Config struct {
Units UnitBackend
Machines MachineBackend
Magic string
}
…the scope of the type’s responsibilities is immediately clearer: the capabilities exposed to it are immediately clearer; and it’s also easier to build more modular test scaffolding around particular areas.
Separate Interfaces From Implementations
If you see an interface defined next to a type that implements that interface, it’s probably wrong; if you see a constructor that returns an interface instead of a concrete type, it’s almost certainly wrong. That is to say:
func NewThingWorker() (*ThingWorker, error)
…is much better than:
func NewThingWorker() (worker.Worker, error)
…even if ThingWorker only exposes Worker methods. This is because interfaces are designed to hide behaviour, but we don’t want to hide behaviour from our creator – she needs to know everything we can do, and she’s responsible for packaging up those capabilities neatly and delivering them where they’re needed.
If the above is too abstract: don’t define an interface next to an implementation of that interface, otherwise you force people to import the implementation when all they need is the interface… (or, they just take the easier route and define their own interface, rendering yours useless).
Aside: you will occasionally find yourself needing factory types or even funcs, which exist purely to abstract the construction of concrete types. These are not quite the same as constructors, so the advice doesn’t apply; and it’s boring but easy to wrap a constructor to return an interface anyway, and these sorts of funcs are verifiable by inspection:
func NewFacade(apiCaller base.APICaller) (Facade, error) {
facade, err := apithing.NewFacade(apiCaller)
if err != nil {
return nil, errors.Trace(err)
}
return facade, nil
}
func NewWorker(config Config) (worker.Worker, error) {
worker, err := New(config)
if err != nil {
return nil, errors.Trace(err)
}
return worker, nil
}
…to the point where you’ll often see this sort of func deployed
alongside a dependency.Manifold, tucked away in an untested shim.go to satisfy the higher goal of rendering the manifold functionality testable in complete isolation.
Super-Awesome Interfaces Are Defined On Their Own
If you’ve created an interface, or a few, with the pure clarity of, say, io.Reader et al, it’s probably worth building a package around them. It’d be a good place for funcs that manipulate those types, and for documentation and discussion of best practice when implementing… and it might even have a couple of useful implementations…
But you’re not going to write something as generally useful as
io.Reader – or, at least, you’re not going to get it right first
time. It’s like pattern mining: when a whole bunch of packages declare the same interfaces with the same semantics, that’s a strong indication that it’s a super-awesome abstraction and it might be a good idea to promote it.
One implementation, one client, and a vision of the future? Not reason enough. Let the client define the interface, lightly shim if necessary, and let the implementation remain ignorant of what hoops it’s jumping through.
Awesome Interfaces Return Interfaces
…or fully-exported structs, at any rate. But the moment you define an interface method to return anything that can’t be fully swapped out by an alternative implementation, you lose the freedom that interfaces give you. (And kinda miss the point of using interfaces in the first place.)
But wait! Earlier, you said that constructors should return concrete types; I can think of 100 methods that are essentially doing just the same thing a constructor would. Is that really a bad idea?
I’m not sure, but: yes, I think it is. Constructors want to be free funcs that explicitly accept all the context they need; if you later discover a situation where you need to pass around a construction capability, either on a factory interface or just as a factory func that returns an interface, it’s trivial to implement a shim that calls the constructor you need – and which is verifiably correct by inspection.
Hidden-Nil Interface Values Will Ruin Your Day
Smallest self-contained code sample I could think of (would have posted a playground link, but, huh, share button apparently not there):
package main
import (
"fmt"
)
// Things have Names.
type Thing interface {
Name() string
}
// thing is a Thing.
type thing struct {
name string
}
func (thing *thing) Name() string {
return thing.name
}
// SafePrint prints Thing Names safely. (Har har.)
func SafePrint(thing Thing) {
if thing != nil {
fmt.Println(thing.Name())
} else {
fmt.Println("oops")
}
}
func main() {
var thing0 = &thing{"bob"}
var thing1 Thing
var thing2 *thing
SafePrint(thing0) // not nil, fine
SafePrint(thing1) // nil, but fine
SafePrint(thing2) // nil pointer, but non-nil interface: panic!
}
This is a strong reason to be unimaginatively verbose and avoid directly returning multiple results – for example, if you’re writing a shim for:
func New() (*ThingWorker, error)
… to expose it as a worker.Worker:
func NewWorker() (worker.Worker, error) {
return New()
}
…is a minor unexploded bomb just waiting for someone to make decisions based on w != nil rather than err == nil and, yay, panic. And, sure, they shouldn’t do that: but you shouldn’t introduce hidden nils. Always just go with the blandly verbose approach:
func NewWorker() (worker.Worker, error) {
worker, err := New()
if err != nil {
return nil, errors.Trace(err)
}
return worker, nil
}
…and, honestly, don’t even take the shortcut when you’re “sure” the func you’re calling returns an interface. People change things, and the compiler won’t protect you when someone fixes that constructor, far from your code.
And for the same reason, never declare a func returning a concrete error type, it’s ludicrously likely to get missed and wreck someone’s day. Probably your own.
Don’t Downcast Or Type-Switch Or Reflect
…unless you really have to. And even then, look into what it’d take to rework your context to avoid it; and if/when defeated, be painfully pedantic about correctness. default: on every switch, , ok on every cast, and be prepared to spend several lines of code for every operation on a reflected value.
(OK, there are legitimate uses for all these things – but they’re infrastructure-y as anything, and intrinsically hairy and hard to understand; and really really best just avoided until you’re backed into a corner that nobody can help you out of. Thanks.)