Code review reference

If you are reading this because I've linked you here: the relevant section contains a canonical version of some shorter code review comment, and I'd be grateful if you reviewed it. Otherwise: hello and welcome to a list of things I commonly say in code reviews. (Hat tip to Eric Breck, who maintains a magnificent document in this form.)

"Your near-synonymous variable names suggest that something is wrong."

If, in the same function / file / class / whatever, you have size and object_size, error and mistake, dimension and dim, or similar, one of these problems is probably happening:

1. Those two variables should be one. (Perhaps this is quick prototype code that hasn't been cleaned up yet.) The best fix is probably to collapse the two variables into one and adjust as appropriate.
2. One of those variables is misnamed. Often, this seems to result from an unwillingness to admit that something needs to be represented at all. Perhaps you think you should have cleaned up the raw input corresponding to X by now but for whatever reason can't, but you don't want to say raw_input_for_X. The best fix--honesty being the best policy in code as elsewhere--is to rename the variables to describe accurately what they represent. You might also need to file a ticket describing the work that would need to be done to make the embarrassing thing unnecessary. (It's OK!)
3. Those variables represent the same underlying thing in different contexts. (Perhaps this is a system that's grown larger than planned.) The best short-term fix is, again, to be more accurate about what the variables represent, even if it's embarrassing. In the longer term, planning in terms of bounded contexts might help.

"You're using a custom type or object here and keeping the definition in your head."

Lists, tuples, and arrays make it easy to pass around related bits of information. Be careful not to rely on the sequential structure of these objects in a way that is semantically opaque. This is a common source of unnecessary coupling between components and of structure living in a programmer's head that should be living in code.

So, if you need to pass around an event's time and place, representing this information as [foo_time, foo_place] and making every user retrieve the time with foo_information[0] and the place with foo_information[1] creates an unnecessary burden. Make an EventCircumstances or EventStamp object (using a struct, a Python dataclass, a Java @AutoValue class, or any other appropriate mechanism). If you really want to represent that data with a list, feel free to do so under the hood, so long as distant callers don't need to know that "[0]" is how you say ".place" in this context.

"This name is not semantically correct."

Prefer to name things according to what they are, rather than the circumstances of their use. If you have an object representing a pizza, and you order pizza only on Tuesdays, name it pizza, not food_we_order_on_tuesday. Some common manifestations of this error are:

1. Text fields. If the field is the serial number but is used by the header of the login screen, call it serial_number, not login_screen_header_text.
2. Exceptions. Exceptions are tricky, because you tend to want context (in a way that makes contextual information part of what the thing is), but still: If the exception arises from a failed widget validation, and that validation happens in your Jenkins CI, call it a WidgetValidationException, not a JenkinsWidgetCheckException.

"This layer of indirection is not permitting any useful abstraction."

Let's say you're representing a light bulb's brightness. Right now you're representing it as a single number of lumens. If you decide to represent that as a Brightness with a single field: sure, let's abstract away from a single primitive data type, for all the usual reasons, including extensibility.

Now let's say you have a Lightbulb object that has LightbulbProperties (the only one of which is the Brightness). Here I'd worry not only that Lightbulb and LightbulbProperties are too nearly synonymous (see above) but that the extra wrapper around this single number of lumens isn't helping.

And if the Lightbulb has LightbulbProperties that has a LightbulbBrightnessProperties that has a Brightness, you almost certainly have more indirection than actual abstraction. Remove at least one of these layers.

This issue commonly arises with central objects in a system, which programmers often feel (justifiedly) hesitant to create and modify directly. Jumping through the layers' definitions in an IDE can feel like watching someone put on latex glove after latex glove after latex glove in the hopes of not leaving a smudge on the vase they're about to handle. I respect the caution but would rather you be able to use your hands.

"I don't think this thing you're trying to formalize can be formalized."

See here.