antirez 2899 days ago. 443150 views. For quite some time I’ve wanted to record a new video talking about code comments for my "writing system software" series on YouTube. However, after giving it some thought, I realized that the topic was better suited for a blog post, so here we are. In this post I analyze Redis comments, trying to categorize them. Along the way I try to show why, in my opinion, writing comments is of paramount importance in order to produce good code, that is maintainable in the long run and understandable by others and by the authors during modifications and debugging activities.

Not everybody thinks likewise. Many believe that comments are useless if the code is solid enough. The idea is that when everything is well designed, the code itself documents what the code is doing, hence code comments are superfluous. I disagree with that vision for two main reasons:

1. Many comments don't explain what the code is doing. They explain what you can't understand just from what the code does. Often this missing information is why the code is doing a certain action, or why it’s doing something that is clear instead of something else that would feel more natural.

2. While it is not generally useful to document, line by line, what the code is doing, because it is understandable just by reading it, a key goal in writing readable code is to lower the amount of effort and the number of details the reader should take into her or his head while reading some code. So comments can be, for me, a tool for lowering the cognitive load of the reader.

The following code snippet is a good example of the second point above. Note that all the code snippets in this blog post are obtained from the Redis source code. Every code snipped is presented prefixed by the file name it was extracted from. The branch used is the current "unstable" with hash 32e0d237.

scripting.c: /* Initial Stack: array / lua_getglobal(lua,"table"); lua_pushstring(lua,"sort"); lua_gettable(lua,-2); / Stack: array, table, table.sort / lua_pushvalue(lua,-3); / Stack: array, table, table.sort, array / if (lua_pcall(lua,1,0,0)) { / Stack: array, table, error */

/* We are not interested in the error, we assume that the problem is

• that there are 'false' elements inside the array, so we try
• again with a slower function but able to handle this case, that
• is: table.sort(table, __redis__compare_helper) / lua_pop(lua,1); / Stack: array, table / lua_pushstring(lua,"sort"); / Stack: array, table, sort / lua_gettable(lua,-2); / Stack: array, table, table.sort / lua_pushvalue(lua,-3); / Stack: array, table, table.sort, array / lua_getglobal(lua,"__redis__compare_helper"); / Stack: array, table, table.sort, array, __redis__compare_helper */ lua_call(lua,2,0); }

Lua uses a stack based API. A reader following each call in the function above, having also a Lua API reference at hand, will be able to mentally reconstruct the stack layout at every given moment. But why to force the reader to do such effort? While writing the code, the original author had to do that mental effort anyway. What I did there was just to annotate every line with the current stack layout after every call. Reading this code is now trivial, regardless of the fact the Lua API is otherwise non trivial to follow.

My goal here is not just to offer my point of view on the usefulness of comments as a tool to provide a background that is not clearly available reading a local section of the source code. But also to also provide some evidence about the usefulness of the kind of comments that are historically considered useless or even dangerous, that is, comments stating what the code is doing, and not why.

Classification of comments

The way I started this work was by reading random parts of the Redis source code, to check if and why comments were useful in different contexts. What quickly emerged was that comments are useful for very different reasons, since they tend to be very different in function, writing style, length and update frequency. I eventually turned the work into a classification task.

During my research I identified nine types of comments:

• Function comments
• Design comments
• Why comments
• Teacher comments
• Checklist comments
• Guide comments
• Trivial comments
• Debt comments
• Backup comments

The first six are, in my opinion, mostly very positive forms of commenting, while the final three are somewhat questionable. In the next sections each type will be analyzed with examples from the Redis source code.

FUNCTION COMMENTS

The goal of a function comment is to prevent the reader from reading code in the first place. Instead, after reading the comment, it should be possible to consider some code as a black box that should obey certain rules. Normally function comments are at the top of functions definitions, but they may be at other places, documenting classes, macros, or other functionally isolated blocks of code that define some interface.

rax.c:

/* Seek the grestest key in the subtree at the current node. Return 0 on

• out of memory, otherwise 1. This is an helper function for different
• iteration functions below. */ int raxSeekGreatest(raxIterator *it) { ...

Function comments are actually a form of in-line API documentation. If the function comment is written well enough, the user should be able most of the times to jump back to what she was reading (reading the code calling such API) without having to read the implementation of a function, a class, a macro, or whatever.

Among all the kinds of comments, these are the ones most widely accepted by the programming community at large as needed. The only point to analyze is if it is a good idea to place comments that are largely API reference documentation inside the code itself. For me the answer is simple: I want the API documentation to exactly match the code. As the code is changed, the documentation should be changed. For this reason, by using function comments as a prologue of functions or other elements, we make the API documentation close to the code, accomplishing three results:

• As the code is changed, the documentation can be easily changed at the same time, without the risk of making the API reference stale.

• This approach maximizes the probability that the author of the change, that should be the one better understanding the change, will also be the author of the API documentation change.

• Reading the code is handy to find the documentation of functions or methods directly where they are defined, so that the reader of the code can focus solely on the code, instead of context switching between code and documentation.

DESIGN COMMENTS

While a "function comment" is usually located at the start of a function, a design comment is more often located at the start of a file. The design comment basically states how and why a given piece of code uses certain algorithms, techniques, tricks, and implementation. It is an higher level overview of what you'll see implemented in the code. With such background, reading the code will be simpler. Moreover I tend to trust more code where I can find design notes. At least I know that some kind of explicit design phase happened, at some point, during the development process.

In my experience design comments are also very useful in order to state, in case the solution proposed by the implementation looks a bit too trivial, what were the competing solutions and why a very simple solution was considered to be enough for the case at hand. If the design is correct, the reader will convince herself that the solution is appropriate and that such simplicity comes from a process, not from being lazy or only knowing how to code basic things.

bio.c:

• DESIGN

• ---

• The design is trivial, we have a structure representing a job to perform
• and a different thread and job queue for every job type.
• Every thread waits for new jobs in its queue, and process every job
• sequentially. ...

WHY COMMENTS

Why comments explain the reason why the code is doing something, even if what the code is doing is crystal clear. See the following example from the Redis replication code.

replication.c:

if (idle > server.repl_backlog_time_limit) { /* When we free the backlog, we always use a new

• replication ID and clear the ID2. This is needed
• because when there is no backlog, the master_repl_offset
• is not updated, but we would still retain our replication
• ID, leading to the following problem:
• 1. We are a master instance.
• 2. Our replica is promoted to master. It's repl-id-2 will
• be the same as our repl-id.
• 3. We, yet as master, receive some updates, that will not
• increment the master_repl_offset.
• 4. Later we are turned into a replica, connect to the new
• master that will accept our PSYNC request by second
• replication ID, but there will be data inconsistency
• because we received writes. */ changeReplicationId(); clearReplicationId2(); freeReplicationBacklog(); serverLog(LL_NOTICE, "Replication backlog freed after %d seconds " "without connected replicas.", (int) server.repl_backlog_time_limit); }

If I check just the function calls there is very little to wonder: if a timeout is reached, change the main replication ID, clear the secondary ID, and finally free the replication backlog. However what is not exactly clear is why we need to change the replication IDs when freeing the backlog.

Now this is the kind of thing that happens continuously in software once it has reached a given level of complexity. Regardless of the code involved, the replication protocol has some level of complexity itself, so we need to do certain things in order to make sure that other bad things can't happen. Probably these kind of comments are, in some way, opportunities to reason about the system and check if it should be improved, so that such complexity is no longer needed, hence also the comment can be removed. However often making something simpler may make something else harder or is simply not viable, or requires future work breaking backward compatibility.

Here is another one.

replication.c:

/* SYNC can't be issued when the server has pending data to send to

• the client about already issued commands. We need a fresh reply
• buffer registering the differences between the BGSAVE and the current
• dataset, so that we can copy to other replicas if needed. */ if (clientHasPendingReplies(c)) { addReplyError(c,"SYNC and PSYNC are invalid with pending output"); return; }

If you run SYNC while there is still pending output (from a past command) to send to the client, the command should fail because during the replication handshake the output buffer of the client is used to accumulate changes, and may be later duplicated to serve other replicas connecting while we are already creating the RDB file for the full sync with the first replica. This is the why we do that. What we do is trivial. Pending replies? Emit an error. Why is rather obscure without the comment.

One may think that such comments are needed only when describing complex protocols and interactions, like in the case of replication. Is that the case? Let's change completely file and goals, and we see still such comments everywhere.

expire.c:

for (j = 0; j < dbs_per_call && timelimit_exit == 0; j++) { int expired; redisDb *db = server.db+(current_db % server.dbnum);

/* Increment the DB now so we are sure if we run out of time

• in the current DB we'll restart from the next. This allows to
• distribute the time evenly across DBs. */ current_db++; ...

That's an interesting one. We want to expire keys from different DBs, as long as we have some time. However instead of incrementing the “database ID” to process next at the end of the loop processing the current database, we do it differently: we select the current DB in the db variable, but then we immediately increment the ID of the next database to process (at the next call of this function). This way if the function terminates because too much effort was spent in a single call, we don't have the problem of restarting again from the same database, letting logically expired keys accumulating in the other databases since we are too focused in processing the same database again and again.

With such comment we both explain why we increment at that stage, and that the next person going to modify the code, should preserve such quality. Note that without the comment the code looks completely harmless. Select, increment, go to do some work. There is no evident reason for not relocating the increment at the end of the loop where it could look more natural.

Trivia: the loop increment was indeed at the end in the original code. It was moved there during a fix: at the same time the comment was added. So let's say this is kinda of a "regression comment".

TEACHER COMMENTS

Teacher comments don't try to explain the code itself or certain side effects we should be aware of. They teach instead the domain (for example math, computer graphics, networking, statistics, complex data structures) in which the code is operating, that may be one outside of the reader skills set, or is simply too full of details to recall all them from memory.

The LOLWUT command in version 5 needs to display rotated squares on the screen (http://antirez.com/news/123). In order to do so it uses some basic trigonometry: despite the fact that the math used is simple, many programmers reading the Redis source code may not have any math background, so the comment at the top of the function explains what's going to happen inside the function itself.

lolwut5.c:

/* Draw a square centered at the specified x,y coordinates, with the specified

• rotation angle and size. In order to write a rotated square, we use the
• trivial fact that the parametric equation:
• x = sin(k)
• y = cos(k)
• Describes a circle for values going from 0 to 2*PI. So basically if we start
• at 45 degrees, that is k = PI/4, with the first point, and then we find
• the other three points incrementing K by PI/2 (90 degrees), we'll have the
• points of the square. In order to rotate the square, we just start with
• k = PI/4 + rotation_angle, and we are done.
• Of course the vanilla equations above will describe the square inside a
• circle of radius 1, so in order to draw larger squares we'll have to
• multiply the obtained coordinates, and then translate them. However this
• is much simpler than implementing the abstract concept of 2D shape and then
• performing the rotation/translation transformation, so for LOLWUT it's
• a good approach. */

The comment does not contain anything that is related to the code of the function itself, or its side effects, or the technical details related to the function. The description is only limited to the mathematical concept that is used inside the function in order to reach a given goal.