Say What You Mean, Not What Happens to Work
Apparently Donald Knuth has said that
Programming is the art of telling another human what one wants the computer to do.
which carries a lot of significance. The thing is that code can be rather easy to write, but it is goddamned hard to read. If we don't remind ourselves periodically that other humans are going to read what we write, we risk writing things that work in the sense that the computer understands it, but it doesn't communicate our intentions to other humans very well.
One or Many?
I recently uncovered the following JavaScript / jQuery snippet.
var profileImages = $('img[id$=profile-image]');
if (profileImages.length > 0) {
var profileImage = profileImages.first();
/* code manipulating profileImage */
}
Still, I'm not entirely sure what the code is supposed to do. The first line gets all profileImages
on the page, and the next line makes sure that the following operations are only performed if there are any profileImages
on the page. That makes a lot of sense. What doesn't is that the line after that extracts just one of the images and performs the operations on that. The other profileImages
are ignored.
Did the author really mean to ignore all but the first image? If so, I suggest using this code instead:
var profileImages = $('img[id$=profile-image]');
console.assert(profileImages.length <= 1, 'The page should have at most one profile-image!');
var profileImage = profileImages.first();
if (profileImage.length) {
/* code manipulating profileImage */
}
This makes it absolutely obvious that you are only interested in one image and gives the reader a good idea of why that is. It still provides a reasonable fallback when there are more than one image, but more importantly, it doesn't sweep the error under the rug. It actually reports that something went wrong. This won't affect any users, but it makes a big difference to the people who wants to understand the code, or troubleshoot why the page isn't behaving as expected.
On the other hand, if you didn't mean to ignore all but one profileImage
, let me suggest performing the operations on all of them.
var profileImages = $('img[id$=profile-image]');
profileImages.each(function() {
var profileImage = $(this);
/* code manipulating profileImage */
});
This will properly do nothing if there are no profileImages
on the page, but otherwise it will perform the operations on all profileImages
.
But It Works?...
"Don't fix what isn't broken" is sometimes used as a defence for writing code like the first version I presented. Please do not ever do that. Take a cue from Knuth. Writing software is something you do as much for people as you do it for the computer. When you are writing software for people, you have to be very clear with what your code says about your intentions. Some people call it self-documenting code, which I think is a bad term because it makes it sound like you don't need real documentation, but in any case, that's what I'm talking about.
As much as possible, it should be obvious from the code not only what the code does, but also what the code is supposed to do.