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.)
If, in the same function / file / class / whatever, you have
dim, or similar, one of these problems is probably happening:
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!)
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_place] and making every user retrieve the time with
foo_information and the place with
foo_information creates an unnecessary burden.
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 "
" is how you say "
.place" in this context.
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
food_we_order_on_tuesday. Some common manifestations of this error are:
WidgetValidationException, not a
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
LightbulbProperties are too nearly synonymous (see above) but that the extra wrapper around this single number of lumens isn't helping.
And if the
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.
More posts about programming