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 size
and object_size
, error
and mistake
, dimension
and 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_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.
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:
serial_number
, not login_screen_header_text
.WidgetValidationException
, not a JenkinsWidgetCheckException
.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.
See here.