Why is assignment in while statement discouraged in Javascript?

Question

I just coded the following loop which works just fine:

let position = 0;
// tslint:disable-next-line:no-conditional-assignment
while ((position = source.indexOf('something interesting', position)) > 0) {
    // do something interesting with something interesting
  position += 1;
}

But note I was obliged to add the tslint exception. Apparently somebody has decided that it is undesirable to allow assignment statements inside the while conditional. Yet this is the most direct code pattern I know for this particular situation.

If this is not "allowed" (or maybe I should say "discouraged") then what do the people driving this think the correct code pattern is?

Unbecoming

They also can be an indicator of overly clever code which decreases maintainability. - Gotta say, what you have written there makes my eyes bleed and I've been at this for a while. palantir.github.io/tslint/rules/no-conditional-assignment – Joinville

I don't know about correct code pattern, but the following will make your co-workers hate you (a little) less: let position = 0; while(true) { if(source.indexOf('something interesting', position) <= 0) break; // do something interesting with something interesting position+=1; } – Joinville

@Adam I have been using this code pattern since the early '70s with the first C compilers that were available on a PDP-11. The syntax was written into K&R for a reason: it is concise and useful. – Unbecoming

Answer 1

It makes the intent of the code harder to determine. Using assignment as an expression is unusual - if you see it inside a condition, how sure are you that the writer of the code intended to use the assignment, or might it be a typo? What if the writer of the code intended to compare the value? EG

while ((position = source.indexOf('something interesting', position)) > 0) {

vs

while ((position == source.indexOf('something interesting', position)) > 0) {

Those two are very different. (In this particular case, Typescript will warn you that the comparison against the boolean and number is probably a mistake, but that won't happen with Javascript, nor in some other situations.)

Rather than assigning inside the conditional, assign outside of it, or even better, for the code above, you could probably use a regular expression (though it's impossible to say for sure without knowing the contents of // do something interesting with something interesting).

It looks repetitive, unfortunately, but an alternative is

let position = 0;
const getPos = () => {
  position = source.indexOf('something interesting', position);
};
getPos();
while (position > 0) {
  // do something interesting with something interesting
  getPos();
}

Filch

Your second example will not even compile in Typescript. I think it is a reasonable assumption that the writer of the code did not intend for it not to compile. – Unbecoming

These sorts of patterns where you want to run something and then run the body of a loop if that something is truthy are the one place where assignment inside a conditional can make sense. It also occurs when iterating over a string with a global regular expression when you also need each match's capture groups (at least until .matchAll becomes usable). Disabling the rule could well sometimes make sense if it makes the code more elegant, but usually there's a better pattern you can use instead of assignment inside a conditional. – Filch

@Unbecoming That's because the code is javascript, not Typescript. For Typescript you need to specify the return type of getPos: function getPost():number { ... – Sliding

@Sliding The Typescript compiler is almost always smart enough to be able to determine the return type automatically (and some prefer using that automatic process rather than explicitly specifying it each time - intellisense works either way). Argument types do have to be specified, but if there are no arguments, no argument types are needed either – Filch

@Filch Ah. I misread the comment. He meant the == example wouldn't compile – Sliding

@Filch Typescript will complain that you are trying to do a > comparison on a boolean – Sliding

Answer 2

As in Adam's example in the comments, an alternative with unconditional assignment could look like this:

let position = 0;
while (true) {
  position = source.indexOf('something interesting', position);
  if (position < 0) {
    break;
  }
  // do something interesting with something interesting
  position += 1;
}

The syntax could become even more compact if you disable curly, which enforces braces on if/for/do/while statements even though K&R Ch 3 does not mandate those braces at all*. This invites a good comparison regarding the motivation for these rules in general. TSLint defines its purpose on its homepage:

TSLint is an extensible static analysis tool that checks TypeScript code for readability, maintainability, and functionality errors.

Is it entirely possible to use a brace-free if statement correctly, particularly if you are diligent about indentation and formatting? Of course. However, it is also entirely possible to misuse that feature in a subtle way, like in the docs for curly:

if (foo === bar)
    foo++;
    bar++;

Your question supposes "apparently somebody has decided that it is undesirable", but the more-accurate statement is that this pattern is a significant source of bugs across codebases, enough that the authors of tslint have offered built-in detection and feedback for that particular pattern. Rather than "undesirable", "not 'allowed'", or "discouraged", the pattern is simply rare-enough and misused-enough to have its detection on by default.

As with all rules in tslint, you can configure the default rule set using tslint.json or tslint.yaml, and you and your team may choose to disable rules like curly and no-conditional-assignment. This may be especially useful if this cases is often a false positive in your coding style (as it seems to be), or if you are applying tslint to an existing codebase where that pattern is common and correctly-used. However, I think it is entirely reasonable for a team to claim (as others have done on this question) that the pattern is prone to =/== typos and other errors and that the correct uses are insufficient to justify allowing the pattern in their codebase.

---

*Note: Though K&R doesn't mandate braces, it does note on p56 that "The indentation shows unequivocally what you want, but the compiler doesn't get the message, and associates the else with the inner if. This kind of bug can be hard to find; it's a good idea to use braces when there are nested ifs." This suggests that even the original K&R text is supportive of reasonable style rules or limits beyond what their syntax supports.

Ko

Answer 3

Yet this is the most direct code pattern I know for this particular situation.

Actually, it is not. A more direct and easier to read pattern is the following:

let position = 0;
while (position >= 0) {
    // do something interesting with something interesting
  position = source.indexOf('something interesting', position) + 1;
}

The code above works because indexOf returns -1 if the string is not found.

Sliding

The first pass of the loop does not have a workable value for position. You would have to add an if statement to make this work. – Unbecoming

The first pass of the loop does have a workable for position it's let position = 0 – Sliding

The key to making it work is >= instead of >. > would not have been workable – Sliding

I am afraid you are missing the point. On the first pass of your version of the loop the value of position is 0. That value does not point to a position in the source string that has the letters 'something interesting'. In fact the source string may not have that pattern at all and yet your loop will execute at least once. So in order for whatever logic in the loop to work properly it will have to handle that special case differently. In other words an additional test that would otherwise not be necessary. – Unbecoming

@Unbecoming is it really not a bug that this code skips over the case where the interesting string occurs at the very start of source? – Geraldo