Strange while loop

[glenak]

Ok, I've never seen a while loop like this. Please can someone help explain this:

CipherInputStream cis = new CipherInputStream(new FileInputStream(new File("plaintext.txt")),cipher);
FileOutputStream fos = new FileOutputStream(new File("ciphertext.txt"));
byte[] cipherTextBytes = new byte[8];
int i = 0;
while((i=cis.read(cipherTextBytes))!= -1) {
fos.write(cipherTextBytes,0,i);
}

Ignore the whole CipherInputStream object if you don't understand it. Basically it's for cryptography. Just look at the while loop. I saw this in a textbook and it actually compiles. cis.read() method is of type int and will return -1 once there's no more byte to read, so I think I get why there's a "= -1" in the while loop. What I don't get is why there's a "i=cis.read()".

[tong1]

The reason is: the loop body still needs i.
the (i=cis.read(cipherTextBytes)) is consider as the returning value. If the value returned is not -1 the loop continues.

[jon.kiparsky]

Paging Douglas Hofstadter... (sorry)
This is a C-type trick. Read it in pieces. While something isn't equal to -1, do the body of this loop. What's being compared to -1? it's

(i=cis.read(cipherTextBytes))

Okay, that's an assignment to i. The cis object is being asked for a read on "cipherTextBytes", and that value is being assigned to "i", which is an int. That's convenient, because the result of that assignment - which is the same as the value being assigned - is being compared to an int, the constant -1.

So: while the output of this read operation is not -1, fos is asked to write the ciphertextBytes - notice that the output of the read is used as a parameter for the write operation.

This is a pseudo-efficiency. It looks cool, because other people can't understand it without squinting and mumbling. It's pseudo-efficiency because other people can't understand it easily - it obscures the intent of the code without actually compiling any more efficiently.

Don't code like this!

[glenak]

Jon, I don't want to code like that. Is there an alternative way?

[jon.kiparsky]

The problem with this:

while((i=cis.read(cipherTextBytes))!= -1) {
fos.write(cipherTextBytes,0,i);
}

is that it's too compressed.
Open it up, make each step obvious:

do {
  int i=cis.read(cipherTextBytes);
  if (i != -1) 
    {
       fos.write(cipherTextBytes, 0, i);
    }
} while (i != -1)

This way it's clear what is happening, when it's happening, and when it doesn't happen. It adds a few lines to the source code, but it should compile to just about the same thing.

I'm not sure off the top of my head about the scope of i. I think it should still be in scope when it's checked at the bottom of the loop, but it might not be. If it isn't, then i has to be defined outside the loop, an easy change.

I use a do while because that's what the original C-like code suggested, but it's trivial to rewrite this as a standard while.

[glenak]

Ah, yes! Cheers, jon

[JamesCherrill]

I want to suggest a different answer - I really don't like the repeated logical expression, and overall I don't think its any easier to understand.
IMHO a major reason why it's hard to understand is the silly choice of variable name, and the use of the EOF code value -1. What about:

while((bytesRead=cis.read(cipherTextBytes)) > 0) {
   fos.write(cipherTextBytes,0,bytesRead);
}

[jon.kiparsky]

That's a matter of style, in my view. This certainly works. If I'm coding for comprehensibility, I won't make an assignment in a test condition because (to me) it obscures the intent. But that's a matter of personal preference, or of house style if you're working with others.

If you're going to do this, I'd suggest a little attention to spacing would help:

while( (bytesRead = cis.read(cipherTextBytes)) >= 0) {
   fos.write(cipherTextBytes, 0, bytesRead);
}

It's a minor thing, but opening up the expression and setting off the assignment portion a little helps the eye to parse the whole thing. Again, though, it's personal style - check with your co-writers, if you're working with others, and work out something that you can all read easily.

(also, for equivalence to the original, you need a >=0. I don't know if 0 is a significant return value from this method...)

[JamesCherrill]

I agree about the spacing; I really believe that these things are very imnportant for code clarity, and, having suffered other people's code on a deadline, I'm an absolute believer in clarity.
I didn't look up the zero case, but as the only use of the value was to write that number of bytes to the outout stream, I guessed it didn't matter.
J