Readable Code is Guessable

There is a lot of discourse around writing readable code, as it’s well established that software engineers spend a great deal more time reading code than writing it. Writing code that is easily understandable has lasting benefits: it encourages similar code to be written throughout the codebase, reduces the number of times the wheel is reinvented, and also allows faster velocity in general.

One way to interpret readability is as the principle of least surprise. You should name your methods and document them so that there are as few surprises as possible in the following code. In other words, it should be easy for someone to guess what the function does without looking at the function body or context in which it is used.

A lot of software engineering is creating simple mental models of certain pieces of behavior, functions, or dependencies. Nobody has enough time to fully understand every piece of their stack, so the more easily you can create a mental interface with something you don’t understand the inner workings of, the more easily you can operate as a productive coder.

Example

Context

An example that brought this to mind recently is a piece of code I read which resulted in a long trace through the codebase just to figure out what exactly it did. This callback is passed into code that extracts a frame from a video, and then writes it out to a file. It is for the purpose of collecting these files into a WebVTT-based storyboard for the video.

// This isn't exactly what the FilePathWriter looked like, but it's close enough
type FilePathWriter interface {
    Write(path string, data []byte) error
}

// This is the actual code

// FrameCallbackFn is a function called for each extracted frame to allow additional processing of
// its image data
type FrameCallbackFn func(writer FilePathWriter, absFrameNum int, img []byte) error

Questions, Guesses, and Answers

Upon reading the function, a few questions immediately sprung to my mind:

1. Does this callback occur before or after the image is written?
2. Is the img slice modifiable by the callback?
3. Why is the writer provided? Is this callback responsible for writing out the image?
4. If the callback errors, what happens?

My initial thoughts on those questions were as follows:

1. The callback probably happens before the image is written, so that the image data is used while buffered in memory.
2. The image slice is probably modifiable by the callback.
3. I genuinely had no idea why the writer was provided.
4. If the callback errors, the error is likely logged, but the frame still gets written. The operation doesn’t entirely abort. After all, the callback can be used again to generate the derivative data based on the written image file.

Even though I had a guess on how it operates, I was very unsure. On its own, that is fairly concerning about a little piece of code.

Before reading on, think to yourself how you would assume this callback operates?

As it turns out, these questions have counterintuitive answers:

1. The callback occurs just after the image is written.
2. The image slice is safe to modify by the callback, but that was not obvious. The image data is written out asynchronously, but before the asynchronous part of the write, the buffer is copied, so the slice is safe to write to.
3. The writer is provided as a way to pass a dependency around. Because this generic callback was created for the specific file to be written to, the writer is used to write the storyboard to the same object storage that the frame files were written to.
4. If the callback errors, the entire operation is aborted. This most likely wasn’t thought through too well, and was a consequence of the classic go idiom if err != nil {return err}.

Improvements

Better Documentation

One lightweight way to improve this callback with minimal friction is to ensure that all questions are documented in the doc comment above the callback.

// FrameCallbackFn is called for each extracted frame to allow additional processing of its image
// data after it is written. Writer allows access to the same object storage that the frame files
// are written to. An error in this callback aborts the entire operation.
type FrameCallbackFn func(writer FilePathWriter, absFrameNum int, img []byte) error

As you can tell, this isn’t actually too helpful, as it relies on documenting weird behavior, and any user of this callback has to keep that behavior in mind. Although we have frontloaded our surprises, they still exist and present a somewhat unpleasant interface.

Refactoring

One thing that stood out first is that we can refactor this code to introduce the object storage writer into the storyboard handler upstream, and avoid doing it at the frame callback level. Because all frames are written into the same storyboard, we can introduce this beforehand and simplify the callback signature.

This results in something that looks vaguely like the below:

type FrameCallbackFn func(absFrameNum int, img []byte) error

// Example code to create the frame callback, where originalCallback is the original storyboard
// callback logic
func createCallback(writer FilePathWriter) FrameCallbackFn {
    return func(absFrameNum int, img []byte) error {
        return originalCallback(writer, absFramNum, img)
    }
}

This is an improvement already, because the user of the callback doesn’t have to think about this detail that was largely introduced for the storyboard usecase.

Another way that we can improve this is by changing the name to specify when in the frame writing process this callback occurs. By changing the name to PostFrameWriteCallback, it is clear when this is called just by assigning it, without having to look at any comments. Additionally, by naming it this way, the fact that mutating the image data does not impact what is written is clear. With this name appropriate documentation looks like:

// PostFrameWriteCallback is called for each extracted frame to allow additional processing of its
// image data after it is written. An error in this callback aborts the entire operation.
type PostFrameWriteCallback func(absFrameNum int, img []byte) error

At this point, some might want to change the error semantics, but I think that it is reasonably unsurprising as a function.

Summary

It’s very important to write unsurprising code. This example was a little egregious, but is an example of a good engineer not thinking about how their code reads to someone who doesn’t know the context. Many things can get missed in the rush to get features and customer needs met, and a little awareness can go a long way.