Code should be written in a way that makes it clear to the reader both what the code is doing, and why it is doing it. This makes reading, editing, and troubleshooting the code a breeze. In fact, troubleshooting is likely to become unnecessary as a side effect of writing code in such a way, because troubleshooting is most often necessary because of human error, and clearly communicating intent in your code reduces chances of human error dramatically.
Example 1
We have a React component that wraps over the Trix WYSIWYG editor[1], and inside we have the following function:
member _.OnEditorReady (editor: Editor) : unit =
editor.element.addEventListener(
"trix-file-accept",
fun event -> event.preventDefault()
)
Do you understand what’s going on? Even for those unfamiliar with F#'s syntax but familiar with React and programming for the web, what’s happening is pretty clear. We have a a member function, OnEditorReady, which is presumably called when the editor is ready (see, we’re building a mental model here, assuming that the name can be trusted, and hypothesizing how the function is used based on its name). Inside it, we add an event listener to the editor’s element (which is presumably the DOM element representing the editor, since we know DOM elements have an addEventListener member function). The event we’re listening to is trix-file-accept, and when such an event occurs, we prevent its default action.
The “what are we doing” is pretty clear. But how about the “why”? Why are we attaching an event listener? What is the default action we’re preventing?
What we’re doing was obvious to us when we were writing the code. After all, the issue we needed to fix was “Trix appears to accept attachments, but they don’t get saved to our backend in any way, so let’s just disable the feature for now”. We then opened Trix docs, found out how to prevent accepting attachments, and implemented it. It was obvious to us, with all this mental context, what this code is meant to accomplish. But to an uninitiated reader, the intent of this code is a mystery. One way to remedy this could be a comment:
member _.OnEditorReady (editor: Editor) : unit =
// prevent Trix from accepting attachments
editor.element.addEventListener(
"trix-file-accept",
fun event -> event.preventDefault()
)
but comments are undesirable for various reasons, so instead we should go for self-documenting code:
let disableAttachments (editor: Editor) : unit =
editor.element.addEventListener(
"trix-file-accept",
fun event -> event.preventDefault()
)
member _.OnEditorReady (editor: Editor) : unit =
disableAttachments editor
This has the additional benefit of preemptively avoiding OnEditorReady from becoming an unorganized dump of code. Now if we need to, say, configure keyboard shortcuts, the no-brainer way to do that would be to repeat the pattern, and end up with
member _.OnEditorReady (editor: Editor) : unit =
disableAttachments editor
configureKeyboardShortcuts editor
Clear, straight-forward, organized, beautiful. The Mona Lisa, as a colleague sarcastically put it :)
Example 2
Consider the following code
member _.ToggleVisibility () : unit =
this.SetState (fun state ->
{ state with IsVisible = true }
)
There’s a serious mismatch between the intent expressed in the name and the actual implementation. The call site may look something like this:
<div OnPress='actions.ToggleVisibility'>Show</div>
<div if='state.IsVisible'>
...
</div>
The reader will no doubt be confused. The button label says “show”, so the expectation is that when pressed, it will show the hidden section. Yet the function called on press is ToggleVisibility, implying “hide if shown, show if hidden”. What’s going on? Is the label incorrect? Is the label correct, but an incorrect function called? Is the function implementation correct, but the name is incorrect?
Depending on what the user experience designer’s intention was in the first place, a number of solutions that clear up the confusion, and correctly communicate intent are possible:
- Change the button label to “toggle”
- Rename the function to
Show - Change the function to
SetVisibility (isVisible: bool)and at the call site have<div OnPress='actions.SetVisibility true'>Show</div>
These solutions offer different tradeoffs, but none of them compromise clear naming and clear expression of intent.
Example 3
Words have both the main meaning, and some finer nuances associated with them. Choose the simplest words with the best fitting nuances to name things in your code. Avoid overloaded words that carry unintended baggage with them.
For example, what do you think of when you see this code:
const metric = {
name = "imageCount",
value = 13,
}
const updatedState = extend(state, {
metricsVault: state.metricsVault.add(userId, metric)
});
When I see metricsVault, I automatically think “vault… sounds like a store, with some kind of security… I wonder why we need security for metrics”. I go to examine the type of state, and see that
type State = {
metricsVault: Map<UserId, Metric>
}
It’s just a simple map! Why not call it metricsStore? Actually, even that would be overkill, since “store” has a nuance of some kind of persistence, but here we just have an in-memory map. Maybe just metrics would suffice, or for better clarity, userIdToMetric?
There are always multiple possible choices. Avoid ones that bring baggage and expectations that do not match reality.
Example 4
What is “temporary” or “obviously a hack” now, upon merging to master becomes a permanent part of the codebase. There’s a good chance it won’t be revisited for weeks, by which point, unless documented, the “temporariness” or “hackyness” of this code will be forgotten.
If you are implementing a hack, make sure the users are well aware of that fact. They can then make an educated choice, in their current context, of whether to use the hack (cheap, but propagates the hack), write a cleaner version local to their use site (more expensive, cleaner, but introduces multiple pieces of code to do the same thing), or rewrite the hack cleanly (most expensive, and potentially has a large retest surface).
In one of the products we were building, a fairly complex layout of DOM elements combined with some complex CSS rules was needed, and the combination resulted in a bug where the change in screen size (when the mobile keyboard is hidden) failed to trigger a page reflow. As a result the UI would get stuck in a rather ugly-looking state.
We investigated the situation thoroughly, and concluded with reasonable certainty that our unique complex layout was hitting up on a bug in the browser rendering engine. There was little we could do to “correctly” fix the problem. So we had to hack like this:
function componentWillReceiveProps(props: Props) : void {
...
setTimeout(function() {
this.forceReflow();
}, 300)
}
This version is pretty horrible in terms of communicating intent. It tells the future users of this code nothing about what is necessitating the reflow. A better version looks like this:
// When screen size changes, given our complex layout, some
// browser versions fail to reflow the page and it gets stuck
// in an ugly state. Forcing a reflow fixes this.
function hackForceReflow() : void {
runLater(300, function() {
this.forceReflow();
})
}
function componentWillReceiveProps(props: Props) : void {
...
this.hackForceReflow();
}
Consider the call site in this iteration. It’s still clear what we’re doing — “forcing a reflow”. It’s also clear that it’s a hack of some sort, and should be treated with care. When we navigate to the function, we see a comment describing why it’s necessary.
We stated that you should not write comments for the wrong reasons. Sometimes there are right reasons to write comments. Given that programming languages have rather limited expressivity compared to natural language, sometimes you will write code that just cannot document itself. Nothing in the value/function naming can communicate the undesired but tricky intent that the code addresses (within reasonable verbosity limits). This is a good use case for a comment. Clear, concise, straight-forward, your prose should be as polished as your code. Step away, clear your mental context, and re-read the comment. Will somebody who does not know this code well be able to read the comment and understand what is going on?
Another example when comments are reasonable is when the concerns that you are addressing have to be by their nature spread across multiple files.
Example 5
Proactively avoid common mistakes by communicating clearly. For example, when representing months as integers[2], the common problem is that it’s unclear, without looking at the documentation, whether January is 0 or 1. Many a real-world error stemmed from this. So next time you need to represent a month as an integer, why not encode your convention right in the name:
function getMonthZeroBased() : int {
...
}
const currentMonthZeroBased: int;
You’ve just both saved the users of the codebase and the resulting software from bugs, and proactively reduced uncertainty for anybody working with the code.
Same goes for units:
// Error and uncertainty prone
const refreshPeriod: int;
// Clear and safe
const refreshPeriodMinutes: int;
In summary, whenever you write code, write it in such a way that both the “what you are doing” and the “why you are doing it” are clearly communicated. In most cases, both can be achieved by making good naming choices, which we cover in the next section.
[1] Thank you for Trix, Basecamp!
[2] Integers for months is generally a horrible idea. Union cases, or named constants would be far less error-prone.
Comments
Post a Comment