Code Review #3: Use the debugger to check “working” code
There is a bug (or two) in this code:
protected void compareStreams(InputStream is, InputStream isFromGet)
throws IOException {
byte[] newBuf = new byte[4096];
byte[] oldBuf = new byte[4096];
int len = 0;
while ((len = is.read(newBuf)) != -1) {
boolean flag = false;
if (isFromGet.read(oldBuf) == len) {
flag = true;
newBuf.equals(oldBuf);
}
if (!flag) {
throw new IOException(”New file differs from old one.”);
}
}
isFromGet.read(newBuf);
if (isFromGet.read(newBuf) != -1) {
throw new IOException(”New file differs from old one.”);
}
}
Did you spot any problems? Well maybe you didn’t spot any right away. But use a debugger, and the bugs would quickly become obvious.
-
Look at :
if (isFromGet.read(oldBuf) == len) { flag = true; newBuf.equals(oldBuf); }The results check
newBuf.equals(oldBuf)is completely ignored. So what happens if 2 streams of the same length but with different contents are compared? This method says they are the same! -
But even if the check was
flag = newBuf.equals(oldBuf), it would still be wrong! Using an IDE (or a debugger) quickly discovers thatnewBuf.equals():public boolean equals(Object obj) { return (this == obj); }This is not the same as comparing the contents of the arrays. The correct call is
Arrays.equals(newBuf, oldBuf). And of course after each read the arrays in question should be 0-filled to avoid issues with partially buffers. - Next, what happens if the streams are read at different rates? newBuf and oldBuf will miscompared simply because one stream didn’t have data immediately available to it.
-
Finally,
isFromGet.read(newBuf); if (isFromGet.read(newBuf) != -1) { throw new IOException("New file differs from old one."); }What is that first line doing in there!? This allows ‘
isFromGet‘ stream to be up to 4096 bytes larger than the ‘is‘ stream!
Yecks! So few lines so many major bugs.
Do a line-by-line code review with a debugger, and discover hidden bugs such as these in “perfect” code.
But finally, haven gotten this far… use the (debugged) work of others…
org.apache.commons.io.IOUtils.contentEquals(InputStream, InputStream) is good and correct. Use that.