I was reading some JavaScript code the other day, because, you know, reading someone else’s code gives you insights and inspirations to improve your own, when I came across this function to calculate the maximum element in an array:
function max(a) { for (var i = a.length, maxValue = a[0]; i--; ) { a[i] > maxValue && (maxValue = a[i]); } return maxValue; }
Taking it slowly, first of all, check out the for
loop. For comparison, here’s how I’d write it myself:
var maxValue = a[0]; for (var i = 1; i < a.length; i++) { // code }
In other words, I’d have to create the working variable to store the maximum value found so far, and initialize it to the first element, and then enter a pretty standard loop, making sure to skip over that first element, and counting up. All three clauses of the for
loop syntax are used. Now take another look at the code I was reading. First, it counts down (to zero). That decrement operator statement also acts as the test for the loop to stop (it’s in the second clause). The third clause of the loop is not used at all, and the initialization clause declares and sets up both the index for the loop and the initial value for the maximum. Boom.
(Aside: if the array were empty, in both my code and the original the function would return undefined
, which is perfectly acceptable. There’s no need therefore for some explicit code to check for an empty array.)
And then for the code in the loop. Here’s how I would write it:
if (a[i] > maxValue) {
maxValue = a[i];
}
The original code makes use of the &&
operator in a statement, something you don’t see all that often. It performs the same operation, but as a single line. In essence, it makes use of short-circuiting: if the first part of the expression (a[i] > maxVa
lue
) is false, the second part is not evaluated. If, on the other hand it is true, the second part is evaluated, and the expression as a whole is set to the resulting value. (The statement just throws this result value away, essentially.)
So, some very interesting code, of which I shall probably take on board the for
loop expression, but, because I still find it jarring, I’ll probably continue to not use the &&
operator in statements like that and instead stick to the if
.
(Note: using the ||
operator in statements like this: a = a || [];
, is very idiomatic and you see it everywhere. This example reads, in essence, use a
if it has been defined, otherwise set it to an empty array.)
9 Responses
#1 Craig Stuntz said...
25-May-12 10:43 AMOne should avoid such code, if for no other reason, because it uses patterns which are usually errors:
Even though the code does work, it has two different things (at least!) which would cause anyone skimming it to suspect that it might be broken.
It's also reinventing the wheel, to some degree. I would write this function as:
#2 Bryan Slatner said...
25-May-12 10:49 AMIt is cool, but I would never use it. Any code that makes me pause for a WTF is code that gets refactored :) I had a similar experience, years ago, when I wrote a parser of some sort in Delphi and I made use of BOOLEAN XOR (an operator which Delphi and C/C++ have, but most other languages don't). It confused so many people I abandoned it.
#3 Will Dudziak said...
25-May-12 10:52 AMI have a Masters in Computer Science and have been programming for 14 years...
Don't do it. It's creative code, no doubt, but obvious.
It should be the objective of every developer to write elegant & obvious code. You will not want to take 5 minutes to dissect your code in a year when you have to rework this method and you've forgotten this trick. You also would not want to make another developer suffer through trying to understand what you've written.
Write your code in obvious ways. Never... Never use obscure methods of getting to your end result.
#4 Jason Karns said...
25-May-12 12:06 PMIt's important not to dismiss something simply because it is assumed to cause bugs in your 'dev circle'. Doing assignment as a predicate is idiomatic in PHP and Ruby. As such, to PHP and Ruby devs, using it in JavaScript isn't obscure in the slightest. (Generally, it's reserved for querying which makes it much easier to spot; which is why I wouldn't use it in this particular instance.)
The point is that things may seem obscure when you first encounter them. But they quickly become second nature and actually enhance the readability of code.
#5 julian m bucknall said...
25-May-12 2:32 PMI think @Jason hit the nail on the head, here. There's a whole clarity/legibility of code "law" that we do have to pay attention to. Along with that though is to admit that every language has its idioms. For example, just because JavaScript looks like a C-type language in syntax, doesn't mean you have to limit yourself to code that would work in C (or C++, or C#, or Java, or...). In fact, idioms become so entrenched that they develop their own clarity and readability. The
||
operator idiom is one such for me.Now, having said that, would I go for this terser
for
loop? Possibly, possibly not. I personally still stumble a bit when I read a complexfor
loop like this (that's bloody Delphi for you, spelling it all out). Would I use the && operator in this manner? It's a recognized idiom, so I dare say it'll start creeping into my code, yes. And this code I was reading was chock-a-block full of this kind of code.Cheers, Julian
#6 richard morris said...
25-May-12 9:19 PMI've been a coder for over 30 years ... I've used hacks like this before (eg: using sets to emulate bitwise math in Delphi) but they break my personal benchmark of writing readable code so I don't have to comment it.
Unless there is a good reason for doing it this way, like saving CPU cycles (and this looks like it would waste a few), the programmer is just trying to impress the poor sod who has to maintain his code.
Besides if the code were ever run in an environment without short circuit evaluation is would break the code silently (ie: it would always return the first element of the array).
#7 Alex Danvy said...
26-May-12 3:54 AMA great example of natural obfuscation. Make a little effort on naming and you'll get a winner :-B
#8 Tymek Majewski said...
28-Feb-13 7:54 PMI think this code make simple operations look very complicated. Not cool at all.
Also it's reinventing the wheel which is usually bad.
Both codes don't check if the a is null, which I don't like either.
#9 julian m bucknall said...
01-Mar-13 11:16 AM@Tymek: Agreed, if you are unfamiliar with the idiom, this kind of code looks like mere obfuscation, almost complexity for the sake of complexity. But it is idiom and if you are going to write any amount of JavaScript it behooves you to learn how other people write it, so you're not confused or misled.
To take an example for C#, it would be like saying that lambda expressions are confusing, it's not C# syntax, we should write explicit delegates instead. Sure, when you first come across lambdas they're a jolt, you have to spend time reasoning what gets generated for the underlying code, but after a while, they become second nature. And that's the point about using
&&
in this fashion in JavaScript.True, there might have to be checks for the
a
parameter beingundefined
/null
(or that it's an array, or is array-like), but that's not what this post is about.Cheers, Julian
Leave a response
Note: some MarkDown is allowed, but HTML is not. Expand to show what's available.
_emphasis_
**strong**
[text](url)
`IEnumerable`
* an item
1. an item
> Now is the time...
Preview of response