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.

1. 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!

2. But even if the check was flag = newBuf.equals(oldBuf), it would still be wrong! Using an IDE (or a debugger) quickly discovers that newBuf.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.

3. 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.
4. 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.